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

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

[Kinglong Mee]

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

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

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

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

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

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

[Jaegeuk Kim]

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

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

[Chao Yu]

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

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

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

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

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

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

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

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

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

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

[Jaegeuk Kim]

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