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. Honza > 2013/1/22, Namjae Jeon <linkinjeon@xxxxxxxxx>: > > 2013/1/22, Jan Kara <jack@xxxxxxx>: > >> On Tue 22-01-13 09:45:09, Namjae Jeon wrote: > >>> 2013/1/21, Jan Kara <jack@xxxxxxx>: > >>> > @@ -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@xxxxxxx> > >> SUSE Labs, CR > >> > > -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
