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