Messages in this thread Patch in this message | | | From | Chao Yu <> | Subject | RE: [PATCH] udf: avoid redundant memcpy when writing data in ICB | Date | Mon, 14 Jul 2014 19:14:39 +0800 |
| |
> -----Original Message----- > From: Jan Kara [mailto:jack@suse.cz] > Sent: Monday, July 14, 2014 4:46 PM > To: Chao Yu > Cc: jack@suse.cz; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] udf: avoid redundant memcpy when writing data in ICB > > On Mon 30-06-14 13:37:15, Chao Yu wrote: > > We have already copied the changed data of page to extent area in ICB when call > > ->write_end() of adinicb aops, so we do not need to copy them another time in > > ->writepage() of adinicb aops. SetPageUptodate is redundant too, so it's better > > to remove these redundant codes. > > > > Also it looks more reasonable to set inode dirty after extent area are modified, > > we'd better move mark_inode_dirty() from ->writepage() to ->write_end(). > This doesn't look right. The copy is there because the page can be > modified via mmap as well, not only via buffered write... So if anything,
You're right, I have made a mistake that I have not considered the 'mmap' case, Thank you for reminding me about that. :)
> we may be able to remove the copy in udf_adinicb_write_end() (actually > using directly simple_write_end() for that), not the one in > udf_adinicb_write_begin().
Agreed, valid data within i_size will be copied to ICB cache when we writeback the page by invoking udf_adinicb_writepage, so the copy in udf_adinicb_write_end is redundant. So our patch should be modified like the following one, is this right?
--- fs/udf/file.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/fs/udf/file.c b/fs/udf/file.c index d80738f..a8dc0f1 100644 --- a/fs/udf/file.c +++ b/fs/udf/file.c @@ -105,16 +105,6 @@ static int udf_adinicb_write_end(struct file *file, loff_t pos, unsigned len, unsigned copied, struct page *page, void *fsdata) { - struct inode *inode = mapping->host; - unsigned offset = pos & (PAGE_CACHE_SIZE - 1); - char *kaddr; - struct udf_inode_info *iinfo = UDF_I(inode); - - kaddr = kmap_atomic(page); - memcpy(iinfo->i_ext.i_data + iinfo->i_lenEAttr + offset, - kaddr + offset, copied); - kunmap_atomic(kaddr); - return simple_write_end(file, mapping, pos, len, copied, page, fsdata); } -- 2.0.1.474.g72c7794
> > Honza > > > > Signed-off-by: Chao Yu <chao2.yu@samsung.com> > > --- > > fs/udf/file.c | 12 ++---------- > > 1 file changed, 2 insertions(+), 10 deletions(-) > > > > diff --git a/fs/udf/file.c b/fs/udf/file.c > > index d80738f..3cd0a50 100644 > > --- a/fs/udf/file.c > > +++ b/fs/udf/file.c > > @@ -65,17 +65,7 @@ static int udf_adinicb_readpage(struct file *file, struct page *page) > > static int udf_adinicb_writepage(struct page *page, > > struct writeback_control *wbc) > > { > > - struct inode *inode = page->mapping->host; > > - char *kaddr; > > - struct udf_inode_info *iinfo = UDF_I(inode); > > - > > BUG_ON(!PageLocked(page)); > > - > > - kaddr = kmap(page); > > - memcpy(iinfo->i_ext.i_data + iinfo->i_lenEAttr, kaddr, inode->i_size); > > - mark_inode_dirty(inode); > > - SetPageUptodate(page); > > - kunmap(page); > > unlock_page(page); > > > > return 0; > > @@ -115,6 +105,8 @@ static int udf_adinicb_write_end(struct file *file, > > kaddr + offset, copied); > > kunmap_atomic(kaddr); > > > > + mark_inode_dirty(inode); > > + > > return simple_write_end(file, mapping, pos, len, copied, page, fsdata); > > } > > > > -- > > 1.7.9.5 > > > > > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR
| |