2013/1/21, Jan Kara <jack@xxxxxxx>: > On Sat 19-01-13 11:17:14, Namjae Jeon wrote: >> From: Namjae Jeon <namjae.jeon@xxxxxxxxxxx> >> >> This patch implements extent caching in case of file reading. >> While reading a file, currently, UDF reads metadata serially >> which takes a lot of time depending on the number of extents present >> in the file. Caching last accessd extent improves metadata read time. >> Instead of reading file metadata from start, now we read from >> the cached extent. >> >> This patch considerably improves the time spent by CPU in kernel mode. >> For example, while reading a 10.9 GB file using dd: >> Time before applying patch: >> 11677022208 bytes (10.9GB) copied, 1529.748921 seconds, 7.3MB/s >> real 25m 29.85s >> user 0m 12.41s >> sys 15m 34.75s >> >> Time after applying patch: >> 11677022208 bytes (10.9GB) copied, 1469.338231 seconds, 7.6MB/s >> real 24m 29.44s >> user 0m 15.73s >> sys 3m 27.61s >> >> Signed-off-by: Namjae Jeon <namjae.jeon@xxxxxxxxxxx> >> Signed-off-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx> >> Signed-off-by: Bonggil Bak <bgbak@xxxxxxxxxxx> > Thanks for the patch Namjae. I did a few more changes to the patch. > Please check them whether you think they are OK. > > diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c > index 0cb208e..7e5aae4 100644 > --- a/fs/udf/ialloc.c > +++ b/fs/udf/ialloc.c > @@ -117,10 +117,6 @@ struct inode *udf_new_inode(struct inode *dir, umode_t > mode, int *err) > iinfo->i_lenAlloc = 0; > iinfo->i_use = 0; > iinfo->i_checkpoint = 1; > - memset(&iinfo->cached_extent, 0, sizeof(struct udf_ext_cache)); > - spin_lock_init(&(iinfo->i_extent_cache_lock)); > - /* Mark extent cache as invalid for now */ > - iinfo->cached_extent.lstart = -1; > if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_AD_IN_ICB)) > iinfo->i_alloc_type = ICBTAG_FLAG_AD_IN_ICB; > else if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_SHORT_AD)) > diff --git a/fs/udf/inode.c b/fs/udf/inode.c > index 8494b8c..fb0c4c4 100644 > --- a/fs/udf/inode.c > +++ b/fs/udf/inode.c > @@ -1305,9 +1305,6 @@ static void udf_fill_inode(struct inode *inode, struct > buffer_head *bh) > iinfo->i_lenAlloc = 0; > iinfo->i_next_alloc_block = 0; > iinfo->i_next_alloc_goal = 0; > - memset(&iinfo->cached_extent, 0, sizeof(struct udf_ext_cache)); > - spin_lock_init(&(iinfo->i_extent_cache_lock)); > - iinfo->cached_extent.lstart = -1; > if (fe->descTag.tagIdent == cpu_to_le16(TAG_IDENT_EFE)) { > iinfo->i_efe = 1; > iinfo->i_use = 0; > Hi Jan. > Initialization now happens in udf_alloc_inode(). Also it's not necessary > to initialized cached_extent.epos when lstart == -1 - noone should look at > that. This change is okay. > > @@ -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. Thanks for review and change! > > @@ -2236,8 +2235,7 @@ void udf_update_extent_cache(struct inode *inode, > loff_t estart, > struct udf_inode_info *iinfo = UDF_I(inode); > > spin_lock(&iinfo->i_extent_cache_lock); > - if (pos->bh != NULL) > - /* Increase ref count */ > + if (pos->bh) > get_bh(pos->bh); > memcpy(&iinfo->cached_extent.epos, pos, > sizeof(struct extent_position)); > @@ -2266,4 +2264,3 @@ void udf_clear_extent_cache(struct udf_inode_info > *iinfo) > iinfo->cached_extent.lstart = -1; > } > } > - > diff --git a/fs/udf/super.c b/fs/udf/super.c > index 186adbf..da8ce9f 100644 > --- a/fs/udf/super.c > +++ b/fs/udf/super.c > @@ -134,6 +134,8 @@ static struct inode *udf_alloc_inode(struct super_block > *sb) > ei->i_next_alloc_goal = 0; > ei->i_strat4096 = 0; > init_rwsem(&ei->i_data_sem); > + ei->cached_extent.lstart = -1; > + spin_lock_init(&ei->i_extent_cache_lock); > > return &ei->vfs_inode; > } > > Honza > -- > 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