diff mbox series

[v3,1/5] erofs: introduce erofs_xattr_iter_fixup_aligned() helper

Message ID 20230518024551.123990-2-jefflexu@linux.alibaba.com
State New
Headers
Series erofs: cleanup of xattr handling |

Commit Message

Jingbo Xu May 18, 2023, 2:45 a.m. UTC 
  Introduce erofs_xattr_iter_fixup_aligned() helper where
it.ofs <= EROFS_BLKSIZ is mandatory.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/erofs/xattr.c | 79 +++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 44 deletions(-)

Comments

Gao Xiang May 29, 2023, 7:41 a.m. UTC | #1 
Hi,

On 2023/5/18 10:45, Jingbo Xu wrote:
> Introduce erofs_xattr_iter_fixup_aligned() helper where
> it.ofs <= EROFS_BLKSIZ is mandatory.
> 
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>   fs/erofs/xattr.c | 79 +++++++++++++++++++++---------------------------
>   1 file changed, 35 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index bbfe7ce170d2..b79be2a556ba 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -29,6 +29,28 @@ struct xattr_iter {
>   	unsigned int ofs;
>   };
>   
> +static inline int erofs_xattr_iter_fixup(struct xattr_iter *it)
> +{
> +	if (it->ofs < it->sb->s_blocksize)
> +		return 0;
> +
> +	it->blkaddr += erofs_blknr(it->sb, it->ofs);
> +	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr, EROFS_KMAP);

could we use a new buf interface to init_metabuf at once?

> +	if (IS_ERR(it->kaddr))
> +		return PTR_ERR(it->kaddr);
> +	it->ofs = erofs_blkoff(it->sb, it->ofs);
> +	return 0;
> +}
> +
> +static inline int erofs_xattr_iter_fixup_aligned(struct xattr_iter *it)

Since we're doing cleanup, this name sounds confusing to me
since here the meaning is actually "we don't allow pos >
blksize", IOWs, any pos <= blksize is allowed here, so
'aligned' is not accurate.

Could we think out a better one?

Thanks,
Gao Xiang
Jingbo Xu May 30, 2023, 3:42 a.m. UTC | #2 
On 5/29/23 3:41 PM, Gao Xiang wrote:
> Hi,
> 
> On 2023/5/18 10:45, Jingbo Xu wrote:
>> Introduce erofs_xattr_iter_fixup_aligned() helper where
>> it.ofs <= EROFS_BLKSIZ is mandatory.
>>
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>>   fs/erofs/xattr.c | 79 +++++++++++++++++++++---------------------------
>>   1 file changed, 35 insertions(+), 44 deletions(-)
>>
>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>> index bbfe7ce170d2..b79be2a556ba 100644
>> --- a/fs/erofs/xattr.c
>> +++ b/fs/erofs/xattr.c
>> @@ -29,6 +29,28 @@ struct xattr_iter {
>>       unsigned int ofs;
>>   };
>>   +static inline int erofs_xattr_iter_fixup(struct xattr_iter *it)
>> +{
>> +    if (it->ofs < it->sb->s_blocksize)
>> +        return 0;
>> +
>> +    it->blkaddr += erofs_blknr(it->sb, it->ofs);
>> +    it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr,
>> EROFS_KMAP);
> 
> could we use a new buf interface to init_metabuf at once?

As discussed offline, I think the following unified API is preferred:

```
int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)

{
    if (it->ofs < it->sb->s_blocksize)
        return 0;

    if (nospan && it->ofs != it->sb->s_blocksize) {
	DBG_BUGON(1);
	return -EFSCORRUPTED;
    }

    ...
}
```
Gao Xiang May 30, 2023, 3:47 a.m. UTC | #3 
On 2023/5/30 11:42, Jingbo Xu wrote:
> 
> 
> On 5/29/23 3:41 PM, Gao Xiang wrote:
>> Hi,
>>
>> On 2023/5/18 10:45, Jingbo Xu wrote:
>>> Introduce erofs_xattr_iter_fixup_aligned() helper where
>>> it.ofs <= EROFS_BLKSIZ is mandatory.
>>>
>>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>>> ---
>>>    fs/erofs/xattr.c | 79 +++++++++++++++++++++---------------------------
>>>    1 file changed, 35 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>>> index bbfe7ce170d2..b79be2a556ba 100644
>>> --- a/fs/erofs/xattr.c
>>> +++ b/fs/erofs/xattr.c
>>> @@ -29,6 +29,28 @@ struct xattr_iter {
>>>        unsigned int ofs;
>>>    };
>>>    +static inline int erofs_xattr_iter_fixup(struct xattr_iter *it)
>>> +{
>>> +    if (it->ofs < it->sb->s_blocksize)
>>> +        return 0;
>>> +
>>> +    it->blkaddr += erofs_blknr(it->sb, it->ofs);
>>> +    it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr,
>>> EROFS_KMAP);
>>
>> could we use a new buf interface to init_metabuf at once?
> 
> As discussed offline, I think the following unified API is preferred:
> 
> ```
> int erofs_xattr_iter_fixup(struct xattr_iter *it, bool nospan)
> 
> {
>      if (it->ofs < it->sb->s_blocksize)
>          return 0;
> 
>      if (nospan && it->ofs != it->sb->s_blocksize) {
> 	DBG_BUGON(1);
> 	return -EFSCORRUPTED;
>      }
> 
>      ...
> }
> ```

Yeah, could you send the next version for this?

Thanks,
Gao Xiang

>
diff mbox series

Patch

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index bbfe7ce170d2..b79be2a556ba 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -29,6 +29,28 @@  struct xattr_iter {
 	unsigned int ofs;
 };
 
+static inline int erofs_xattr_iter_fixup(struct xattr_iter *it)
+{
+	if (it->ofs < it->sb->s_blocksize)
+		return 0;
+
+	it->blkaddr += erofs_blknr(it->sb, it->ofs);
+	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr, EROFS_KMAP);
+	if (IS_ERR(it->kaddr))
+		return PTR_ERR(it->kaddr);
+	it->ofs = erofs_blkoff(it->sb, it->ofs);
+	return 0;
+}
+
+static inline int erofs_xattr_iter_fixup_aligned(struct xattr_iter *it)
+{
+	if (it->ofs > it->sb->s_blocksize) {
+		DBG_BUGON(1);
+		return -EFSCORRUPTED;
+	}
+	return erofs_xattr_iter_fixup(it);
+}
+
 static int erofs_init_inode_xattrs(struct inode *inode)
 {
 	struct erofs_inode *const vi = EROFS_I(inode);
@@ -80,6 +102,7 @@  static int erofs_init_inode_xattrs(struct inode *inode)
 		goto out_unlock;
 	}
 
+	it.sb = sb;
 	it.buf = __EROFS_BUF_INITIALIZER;
 	it.blkaddr = erofs_blknr(sb, erofs_iloc(inode) + vi->inode_isize);
 	it.ofs = erofs_blkoff(sb, erofs_iloc(inode) + vi->inode_isize);
@@ -105,19 +128,11 @@  static int erofs_init_inode_xattrs(struct inode *inode)
 	it.ofs += sizeof(struct erofs_xattr_ibody_header);
 
 	for (i = 0; i < vi->xattr_shared_count; ++i) {
-		if (it.ofs >= sb->s_blocksize) {
-			/* cannot be unaligned */
-			DBG_BUGON(it.ofs != sb->s_blocksize);
-
-			it.kaddr = erofs_read_metabuf(&it.buf, sb, ++it.blkaddr,
-						      EROFS_KMAP);
-			if (IS_ERR(it.kaddr)) {
-				kfree(vi->xattr_shared_xattrs);
-				vi->xattr_shared_xattrs = NULL;
-				ret = PTR_ERR(it.kaddr);
-				goto out_unlock;
-			}
-			it.ofs = 0;
+		ret = erofs_xattr_iter_fixup_aligned(&it);
+		if (ret) {
+			kfree(vi->xattr_shared_xattrs);
+			vi->xattr_shared_xattrs = NULL;
+			goto out_unlock;
 		}
 		vi->xattr_shared_xattrs[i] =
 			le32_to_cpu(*(__le32 *)(it.kaddr + it.ofs));
@@ -150,20 +165,6 @@  struct xattr_iter_handlers {
 		      unsigned int len);
 };
 
-static inline int xattr_iter_fixup(struct xattr_iter *it)
-{
-	if (it->ofs < it->sb->s_blocksize)
-		return 0;
-
-	it->blkaddr += erofs_blknr(it->sb, it->ofs);
-	it->kaddr = erofs_read_metabuf(&it->buf, it->sb, it->blkaddr,
-				       EROFS_KMAP);
-	if (IS_ERR(it->kaddr))
-		return PTR_ERR(it->kaddr);
-	it->ofs = erofs_blkoff(it->sb, it->ofs);
-	return 0;
-}
-
 static int inline_xattr_iter_begin(struct xattr_iter *it,
 				   struct inode *inode)
 {
@@ -201,7 +202,7 @@  static int xattr_foreach(struct xattr_iter *it,
 	int err;
 
 	/* 0. fixup blkaddr, ofs, ipage */
-	err = xattr_iter_fixup(it);
+	err = erofs_xattr_iter_fixup(it);
 	if (err)
 		return err;
 
@@ -236,14 +237,9 @@  static int xattr_foreach(struct xattr_iter *it,
 	processed = 0;
 
 	while (processed < entry.e_name_len) {
-		if (it->ofs >= it->sb->s_blocksize) {
-			DBG_BUGON(it->ofs > it->sb->s_blocksize);
-
-			err = xattr_iter_fixup(it);
-			if (err)
-				goto out;
-			it->ofs = 0;
-		}
+		err = erofs_xattr_iter_fixup_aligned(it);
+		if (err)
+			goto out;
 
 		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
 			      entry.e_name_len - processed);
@@ -271,14 +267,9 @@  static int xattr_foreach(struct xattr_iter *it,
 	}
 
 	while (processed < value_sz) {
-		if (it->ofs >= it->sb->s_blocksize) {
-			DBG_BUGON(it->ofs > it->sb->s_blocksize);
-
-			err = xattr_iter_fixup(it);
-			if (err)
-				goto out;
-			it->ofs = 0;
-		}
+		err = erofs_xattr_iter_fixup_aligned(it);
+		if (err)
+			goto out;
 
 		slice = min_t(unsigned int, it->sb->s_blocksize - it->ofs,
 			      value_sz - processed);