> -----Original Message-----
> From: Jan Kara [mailto:[email protected]]
> Sent: Monday, July 14, 2014 4:46 PM
> To: Chao Yu
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
