subject:"\[f2fs\-dev\] \[PATCH\] f2fs\: copy all valid xattr data includes the last zero"

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-22 Thread Chao Yu

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 
>>> ---
>>>  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);
>>> +

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-22 Thread Jaegeuk Kim

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

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-22 Thread Kinglong Mee

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 
>> ---
>>  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 

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);
>> -

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-22 Thread Chao Yu

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.

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 >

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-20 Thread Kinglong Mee

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.

>> +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

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-19 Thread Jaegeuk Kim

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;
> + 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
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

2017-03-17 Thread Kinglong Mee

It's better coping all valid xattr data includes the last zero. 

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;
+   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
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

Re: [f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

[f2fs-dev] [PATCH] f2fs: copy all valid xattr data includes the last zero

7 matches

Site Navigation

Mail list logo

Footer information