* [PATCH] f2fs: copy all valid xattr data includes the last zero @ 2017-03-18 1:23 Kinglong Mee 2017-03-20 3:08 ` Jaegeuk Kim 0 siblings, 1 reply; 10+ messages in thread From: Kinglong Mee @ 2017-03-18 1:23 UTC (permalink / raw) To: Jaegeuk Kim, linux-f2fs-devel It's better coping all valid xattr data includes the last zero. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- 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; + unsigned int size = xnid ? MAX_XATTR_BLOCK_SIZE : 0; unsigned int inline_size = 0; int err = 0; @@ -328,7 +328,7 @@ 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; + size_t size = MAX_XATTR_BLOCK_SIZE, inline_size = 0; void *txattr_addr; int err; diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h index d5a9492..629c8ae 100644 --- a/fs/f2fs/xattr.h +++ b/fs/f2fs/xattr.h @@ -73,9 +73,9 @@ 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)) +/* A __u32 is reserved for the last entry as zero */ #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ - VALID_XATTR_BLOCK_SIZE) + MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) #define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \ sizeof(struct f2fs_xattr_header) - \ -- 2.9.3 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-18 1:23 [PATCH] f2fs: copy all valid xattr data includes the last zero Kinglong Mee @ 2017-03-20 3:08 ` Jaegeuk Kim 2017-03-20 12:33 ` Kinglong Mee 0 siblings, 1 reply; 10+ messages in thread From: Jaegeuk Kim @ 2017-03-20 3:08 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> > --- > 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; > + unsigned int size = xnid ? MAX_XATTR_BLOCK_SIZE : 0; > unsigned int inline_size = 0; > int err = 0; > > @@ -328,7 +328,7 @@ 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; > + size_t size = MAX_XATTR_BLOCK_SIZE, inline_size = 0; > void *txattr_addr; > int err; > > diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h > index d5a9492..629c8ae 100644 > --- a/fs/f2fs/xattr.h > +++ b/fs/f2fs/xattr.h > @@ -73,9 +73,9 @@ 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)) > +/* A __u32 is reserved for the last entry as zero */ > #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ > - VALID_XATTR_BLOCK_SIZE) > + MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) > > #define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \ > sizeof(struct f2fs_xattr_header) - \ > -- > 2.9.3 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-20 3:08 ` Jaegeuk Kim @ 2017-03-20 12:33 ` Kinglong Mee 2017-03-21 16:18 ` Jaegeuk Kim 0 siblings, 1 reply; 10+ messages in thread From: Kinglong Mee @ 2017-03-20 12:33 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> >> --- >> 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. >> + unsigned int size = xnid ? MAX_XATTR_BLOCK_SIZE : 0; >> unsigned int inline_size = 0; >> int err = 0; >> >> @@ -328,7 +328,7 @@ 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; Yes, it's larger. It's for consistent with the above. thanks, Kinglong Mee >> + size_t size = MAX_XATTR_BLOCK_SIZE, inline_size = 0; >> void *txattr_addr; >> int err; >> >> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h >> index d5a9492..629c8ae 100644 >> --- a/fs/f2fs/xattr.h >> +++ b/fs/f2fs/xattr.h >> @@ -73,9 +73,9 @@ 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)) >> +/* A __u32 is reserved for the last entry as zero */ >> #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ >> - VALID_XATTR_BLOCK_SIZE) >> + MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) >> >> #define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \ >> sizeof(struct f2fs_xattr_header) - \ >> -- >> 2.9.3 > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-20 12:33 ` Kinglong Mee @ 2017-03-21 16:18 ` Jaegeuk Kim 2017-03-22 8:42 ` Chao Yu 0 siblings, 1 reply; 10+ messages in thread From: Jaegeuk Kim @ 2017-03-21 16:18 UTC (permalink / raw) To: Kinglong Mee; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> > >> --- > >> 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? Thanks, > >> + unsigned int size = xnid ? MAX_XATTR_BLOCK_SIZE : 0; > >> unsigned int inline_size = 0; > >> int err = 0; > >> > >> @@ -328,7 +328,7 @@ 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; > > Yes, it's larger. > It's for consistent with the above. > > thanks, > Kinglong Mee > > >> + size_t size = MAX_XATTR_BLOCK_SIZE, inline_size = 0; > >> void *txattr_addr; > >> int err; > >> > >> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h > >> index d5a9492..629c8ae 100644 > >> --- a/fs/f2fs/xattr.h > >> +++ b/fs/f2fs/xattr.h > >> @@ -73,9 +73,9 @@ 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)) > >> +/* A __u32 is reserved for the last entry as zero */ > >> #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ > >> - VALID_XATTR_BLOCK_SIZE) > >> + MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) > >> > >> #define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \ > >> sizeof(struct f2fs_xattr_header) - \ > >> -- > >> 2.9.3 > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-21 16:18 ` Jaegeuk Kim @ 2017-03-22 8:42 ` Chao Yu 2017-03-22 10:16 ` Chao Yu 0 siblings, 1 reply; 10+ messages in thread From: Chao Yu @ 2017-03-22 8:42 UTC (permalink / raw) To: Jaegeuk Kim, Kinglong Mee; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> >>>> --- >>>> 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; + 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) -- 2.8.2.295.g3f1c1d0 > > Thanks, > >>>> + unsigned int size = xnid ? MAX_XATTR_BLOCK_SIZE : 0; >>>> unsigned int inline_size = 0; >>>> int err = 0; >>>> >>>> @@ -328,7 +328,7 @@ 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; >> >> Yes, it's larger. >> It's for consistent with the above. >> >> thanks, >> Kinglong Mee >> >>>> + size_t size = MAX_XATTR_BLOCK_SIZE, inline_size = 0; >>>> void *txattr_addr; >>>> int err; >>>> >>>> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h >>>> index d5a9492..629c8ae 100644 >>>> --- a/fs/f2fs/xattr.h >>>> +++ b/fs/f2fs/xattr.h >>>> @@ -73,9 +73,9 @@ 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)) >>>> +/* A __u32 is reserved for the last entry as zero */ >>>> #define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \ >>>> - VALID_XATTR_BLOCK_SIZE) >>>> + MAX_XATTR_BLOCK_SIZE - sizeof(__u32)) >>>> >>>> #define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \ >>>> sizeof(struct f2fs_xattr_header) - \ >>>> -- >>>> 2.9.3 >>> > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > . > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-22 8:42 ` Chao Yu @ 2017-03-22 10:16 ` Chao Yu 2017-03-22 10:26 ` Kinglong Mee 2017-03-22 14:21 ` Jaegeuk Kim 0 siblings, 2 replies; 10+ messages in thread From: Chao Yu @ 2017-03-22 10:16 UTC (permalink / raw) To: Jaegeuk Kim, Kinglong Mee; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> >>>>> --- >>>>> 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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-22 10:16 ` Chao Yu @ 2017-03-22 10:26 ` Kinglong Mee 2017-03-22 14:21 ` Jaegeuk Kim 1 sibling, 0 replies; 10+ messages in thread From: Kinglong Mee @ 2017-03-22 10:26 UTC (permalink / raw) To: Chao Yu, Jaegeuk Kim; +Cc: linux-f2fs-devel On 3/22/2017 18:16, 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 <kinglongmee@gmail.com> >>>>>> --- >>>>>> 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. > > unsigned int size = VALID_XATTR_BLOCK_SIZE; It's better that mine. Reviewed-by: Kinglong Mee <kinglongmee@gmail.com> Thanks, Kinglong Mee > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-22 10:16 ` Chao Yu 2017-03-22 10:26 ` Kinglong Mee @ 2017-03-22 14:21 ` Jaegeuk Kim 2017-03-23 1:22 ` Chao Yu 1 sibling, 1 reply; 10+ messages in thread From: Jaegeuk Kim @ 2017-03-22 14:21 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel 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 <kinglongmee@gmail.com> > >>>>> --- > >>>>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-22 14:21 ` Jaegeuk Kim @ 2017-03-23 1:22 ` Chao Yu 2017-03-23 3:53 ` Jaegeuk Kim 0 siblings, 1 reply; 10+ messages in thread From: Chao Yu @ 2017-03-23 1:22 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel On 2017/3/22 22:21, Jaegeuk Kim wrote: > 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 <kinglongmee@gmail.com> >>>>>>> --- >>>>>>> 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. To cover below case - __f2fs_setxattr - read_all_xattrs - write_all_xattrs - new_node_page - memcpy > > 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); And this needs to be kept to avoid remain random data in newly allocated xattr node page. Thanks, >>> + 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: copy all valid xattr data includes the last zero 2017-03-23 1:22 ` Chao Yu @ 2017-03-23 3:53 ` Jaegeuk Kim 0 siblings, 0 replies; 10+ messages in thread From: Jaegeuk Kim @ 2017-03-23 3:53 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel On 03/23, Chao Yu wrote: > On 2017/3/22 22:21, Jaegeuk Kim wrote: > > 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 <kinglongmee@gmail.com> > >>>>>>> --- > >>>>>>> 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. > > To cover below case > > - __f2fs_setxattr > - read_all_xattrs > - write_all_xattrs > - new_node_page > - memcpy Oh, I see. This happened in your change. Thanks, > > > > > 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); > > And this needs to be kept to avoid remain random data in newly allocated xattr > node page. > > Thanks, > > >>> + 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-23 3:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-18 1:23 [PATCH] f2fs: copy all valid xattr data includes the last zero Kinglong Mee 2017-03-20 3:08 ` Jaegeuk Kim 2017-03-20 12:33 ` Kinglong Mee 2017-03-21 16:18 ` Jaegeuk Kim 2017-03-22 8:42 ` Chao Yu 2017-03-22 10:16 ` Chao Yu 2017-03-22 10:26 ` Kinglong Mee 2017-03-22 14:21 ` Jaegeuk Kim 2017-03-23 1:22 ` Chao Yu 2017-03-23 3:53 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).