linux-kernel - Re: [PATCH] udf: avoid redundant memcpy when writing data in ICB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140714131356.GB17279@quack.suse.cz>
Date:	Mon, 14 Jul 2014 15:13:56 +0200
From:	Jan Kara <jack@...e.cz>
To:	Chao Yu <chao2.yu@...sung.com>
Cc:	'Jan Kara' <jack@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] udf: avoid redundant memcpy when writing data in ICB

On Mon 14-07-14 19:14:39, Chao Yu wrote:
> > -----Original Message-----
> > From: Jan Kara [mailto:jack@...e.cz]
> > Sent: Monday, July 14, 2014 4:46 PM
> > To: Chao Yu
> > Cc: jack@...e.cz; linux-kernel@...r.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);
>  }
  You can completely remove udf_adinicb_write_end() function and just use
simple_write_end() in udf_adinicb_aops. 
 								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/