On Mon 14-07-14 19:14:39, Chao Yu wrote:
> > -----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);
> }
You can completely remove udf_adinicb_write_end() function and just use
simple_write_end() in udf_adinicb_aops.
Honza
--
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/
