lkml.org 
[lkml]   [2013]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] udf: add extent cache support in case of file reading
    From
    2013/2/4, Jan Kara <jack@suse.cz>:
    > On Sat 02-02-13 15:21:09, Namjae Jeon wrote:
    >> Hi. Jan.
    >>
    >> Sorry for interrupt.
    >> Have you taken this patch to your tree ? I can not find it..
    >> or Is there any issue regarding this patch ?
    > I had it in my tree but not in the for_next branch. Did it now so you
    > should see the patch in tomorrow's linux-next.
    Okay, I see.
    Thanks Jan!
    >
    > Honza
    >> 2013/1/22, Namjae Jeon <linkinjeon@gmail.com>:
    >> > 2013/1/22, Jan Kara <jack@suse.cz>:
    >> >> On Tue 22-01-13 09:45:09, Namjae Jeon wrote:
    >> >>> 2013/1/21, Jan Kara <jack@suse.cz>:
    >> >>> > @@ -2222,6 +2219,8 @@ int udf_read_extent_cache(struct inode
    >> >>> > *inode,
    >> >>> > loff_t
    >> >>> > bcount,
    >> >>> > *lbcount = iinfo->cached_extent.lstart;
    >> >>> > memcpy(pos, &iinfo->cached_extent.epos,
    >> >>> > sizeof(struct extent_position));
    >> >>> > + if (pos->bh)
    >> >>> > + get_bh(pos->bh);
    >> >>> > spin_unlock(&iinfo->i_extent_cache_lock);
    >> >>> > return 1;
    >> >>> > } else
    >> >>> > This is the most important - we should give buffer reference to
    >> >>> > pos->bh.
    >> >>> > Caller will eventually free it right?
    >> >>> This change is not required as we give buffer reference to pos->bh at
    >> >>> the time of cache update.
    >> >>> When we start reading a file, first we try to read the cache which
    >> >>> will lead to cache miss.
    >> >>> So, we would really access the pos->bh in udf_update_extent_cache for
    >> >>> the first time, and this is where the buffer reference is
    >> >>> incremented.
    >> >>> Calling get_bh at 2 places will eventually lead to mem leak.
    >> >>> Let me know your opinion.
    >> >> Yes, udf_update_extent_cache() gets its own reference to bh but that
    >> >> is
    >> >> dropped in udf_clear_extent_cache(). So I think
    >> >> udf_read_extent_cache()
    >> >> needs to get a reference to the caller (as the caller will eventually
    >> >> free
    >> >> the bh via brelse(epos.bh) e.g. in udf_extend_file(). Also I realized
    >> >> udf_update_extent_cache() needs to first clear the cache if it is
    >> >> valid.
    >> >> Otherwise it just overwrites bh pointer and reference is leaked. Is it
    >> >> clearer now?
    >> > Yes, you're right. Also, this patch looks good to me.
    >> >>
    >> >> I've also changed locking of udf_clear_extent_cache() so that
    >> >> i_extent_cache_lock is always taken for that function - it makes the
    >> >> locking rules obvious at the first sight.
    >> > Yes, right. it is needed.
    >> > When we test with this patch, working fine.
    >> > Thanks Jan!
    >> >>
    >> >> Attached is the patch I currently carry.
    >> >>
    >> >> Honza
    >> >>
    >> >> --
    >> >> Jan Kara <jack@suse.cz>
    >> >> SUSE Labs, CR
    >> >>
    >> >
    > --
    > Jan Kara <jack@suse.cz>
    > SUSE Labs, CR
    >


    \
     
     \ /
      Last update: 2013-02-04 12:05    [W:3.589 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site