From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaegeuk Kim Subject: Re: [PATCH] f2fs: copy all valid xattr data includes the last zero Date: Wed, 22 Mar 2017 10:21:40 -0400 Message-ID: <20170322142140.GA74218@jaegeuk.local> References: <20170320030842.GB45667@jaegeuk.local> <10384529-5d09-712c-abba-a981be074072@gmail.com> <20170321161830.GB61126@jaegeuk.local> <9ed8366e-28f7-edf4-5078-54638d2c8990@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1cqh97-0008ON-Fx for linux-f2fs-devel@lists.sourceforge.net; Wed, 22 Mar 2017 14:21:53 +0000 Received: from mail.kernel.org ([198.145.29.136]) by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.76) id 1cqh95-0007o5-HB for linux-f2fs-devel@lists.sourceforge.net; Wed, 22 Mar 2017 14:21:53 +0000 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net To: Chao Yu Cc: linux-f2fs-devel@lists.sourceforge.net On 03/22, Chao Yu wrote: > On 2017/3/22 16:42, Chao Yu wrote: > > On 2017/3/22 0:18, Jaegeuk Kim wrote: > >> On 03/20, Kinglong Mee wrote: > >>> On 3/20/2017 11:08, Jaegeuk Kim wrote: > >>>> Hi Kinglong, > >>>> > >>>> On 03/18, Kinglong Mee wrote: > >>>>> It's better coping all valid xattr data includes the last zero. > >>>> > >>>> Why do we need this? > >>>> > >>>> The size of txattr_addr would be larger than the space we need. > >>>> > >>>> Thanks, > >>>> > >>>>> > >>>>> Signed-off-by: Kinglong Mee > >>>>> --- > >>>>> fs/f2fs/xattr.c | 4 ++-- > >>>>> fs/f2fs/xattr.h | 4 ++-- > >>>>> 2 files changed, 4 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > >>>>> index aff7619..41785c9 100644 > >>>>> --- a/fs/f2fs/xattr.c > >>>>> +++ b/fs/f2fs/xattr.c > >>>>> @@ -249,7 +249,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage, > >>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > >>>>> void *cur_addr, *txattr_addr, *last_addr = NULL; > >>>>> nid_t xnid = F2FS_I(inode)->i_xattr_nid; > >>>>> - unsigned int size = xnid ? VALID_XATTR_BLOCK_SIZE : 0; > >>> > >>> Here maybe does not copy the last zero. > >> > >> The below kzalloc() will make it zero. Any problem? > > > > There is no problem without this change, but anyway, IMO, it needs to do cleanup > > as it can provider better readability making allocated size and copied size be > > consistent among lookup_all_xattrs, read_all_xattrs and write_all_xattrs. > > > > And since during allocation we have initialize memory with zero, so I prefer to > > not copy extra reserved xattr space. > > > > Jaegeuk, Kinglong, how about changing like below: > > > > --- > > fs/f2fs/xattr.c | 27 ++++++++++++--------------- > > fs/f2fs/xattr.h | 3 ++- > > 2 files changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > > index aff7619e3f96..308b06664068 100644 > > --- a/fs/f2fs/xattr.c > > +++ b/fs/f2fs/xattr.c > > @@ -250,15 +250,13 @@ static int lookup_all_xattrs(struct inode *inode, struct > > page *ipage, > > void *cur_addr, *txattr_addr, *last_addr = NULL; > > nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > unsigned int size = xnid ? VALID_XATTR_BLOCK_SIZE : 0; > > - unsigned int inline_size = 0; > > + unsigned int inline_size = inline_xattr_size(inode); > > int err = 0; > > > > - inline_size = inline_xattr_size(inode); > > - > > if (!size && !inline_size) > > return -ENODATA; > > > > - txattr_addr = kzalloc(inline_size + size + sizeof(__u32), > > + txattr_addr = kzalloc(inline_size + size + RESERVED_XATTR_SIZE, > > GFP_F2FS_ZERO); > > if (!txattr_addr) > > return -ENOMEM; > > @@ -328,13 +326,14 @@ static int read_all_xattrs(struct inode *inode, struct > > page *ipage, > > { > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > struct f2fs_xattr_header *header; > > - size_t size = PAGE_SIZE, inline_size = 0; > > + nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > + unsigned int size = xnid ? VALID_XATTR_BLOCK_SIZE : 0; > > Here, always should assign size with VALID_XATTR_BLOCK_SIZE for enospc case in > inline xattr space while adding new xattr entry. Do you mean any race condition? It seems setxattr is covered by i_mutex, but listxattr does not. But, given xnid, i_xattr_nid won't be used below, so it looks fine to go with xnid check. Thanks, > > unsigned int size = VALID_XATTR_BLOCK_SIZE; > > Thanks, > > > + unsigned int inline_size = inline_xattr_size(inode); > > void *txattr_addr; > > int err; > > > > - inline_size = inline_xattr_size(inode); > > - > > - txattr_addr = kzalloc(inline_size + size, GFP_F2FS_ZERO); > > + txattr_addr = kzalloc(inline_size + size + RESERVED_XATTR_SIZE, > > + GFP_F2FS_ZERO); > > if (!txattr_addr) > > return -ENOMEM; > > > > @@ -358,19 +357,19 @@ static int read_all_xattrs(struct inode *inode, struct > > page *ipage, > > } > > > > /* read from xattr node block */ > > - if (F2FS_I(inode)->i_xattr_nid) { > > + if (xnid) { > > struct page *xpage; > > void *xattr_addr; > > > > /* The inode already has an extended attribute block. */ > > - xpage = get_node_page(sbi, F2FS_I(inode)->i_xattr_nid); > > + xpage = get_node_page(sbi, xnid); > > if (IS_ERR(xpage)) { > > err = PTR_ERR(xpage); > > goto fail; > > } > > > > xattr_addr = page_address(xpage); > > - memcpy(txattr_addr + inline_size, xattr_addr, PAGE_SIZE); > > + memcpy(txattr_addr + inline_size, xattr_addr, size); > > f2fs_put_page(xpage, 1); > > } > > > > @@ -392,14 +391,12 @@ static inline int write_all_xattrs(struct inode *inode, > > __u32 hsize, > > void *txattr_addr, struct page *ipage) > > { > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > - size_t inline_size = 0; > > + size_t inline_size = inline_xattr_size(inode); > > void *xattr_addr; > > struct page *xpage; > > nid_t new_nid = 0; > > int err; > > > > - inline_size = inline_xattr_size(inode); > > - > > if (hsize > inline_size && !F2FS_I(inode)->i_xattr_nid) > > if (!alloc_nid(sbi, &new_nid)) > > return -ENOSPC; > > @@ -454,7 +451,7 @@ static inline int write_all_xattrs(struct inode *inode, > > __u32 hsize, > > } > > > > xattr_addr = page_address(xpage); > > - memcpy(xattr_addr, txattr_addr + inline_size, MAX_XATTR_BLOCK_SIZE); > > + memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE); > > set_page_dirty(xpage); > > f2fs_put_page(xpage, 1); > > > > diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h > > index d5a94928c116..1e7db8d0806e 100644 > > --- a/fs/f2fs/xattr.h > > +++ b/fs/f2fs/xattr.h > > @@ -73,7 +73,8 @@ struct f2fs_xattr_entry { > > !IS_XATTR_LAST_ENTRY(entry);\ > > entry = XATTR_NEXT_ENTRY(entry)) > > #define MAX_XATTR_BLOCK_SIZE (PAGE_SIZE - sizeof(struct node_footer)) > > -#define VALID_XATTR_BLOCK_SIZE (MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) > > +#define RESERVED_XATTR_SIZE (sizeof(__u32)) > > +#define VALID_XATTR_BLOCK_SIZE (MAX_XATTR_BLOCK_SIZE - RESERVED_XATTR_SIZE) > > #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ > > VALID_XATTR_BLOCK_SIZE) > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot