* Re: [PATCH] erofs: update ctx->pos for every emitted dirent
2022-05-27 7:25 [PATCH] erofs: update ctx->pos for every emitted dirent Hongnan Li
@ 2022-05-28 12:56 ` Gao Xiang
2022-05-29 6:26 ` Chao Yu
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Gao Xiang @ 2022-05-28 12:56 UTC (permalink / raw)
To: Hongnan Li; +Cc: linux-erofs, xiang, chao, linux-kernel
Hi Hongnan,
On Fri, May 27, 2022 at 03:25:36PM +0800, Hongnan Li wrote:
> erofs_readdir update ctx->pos after filling a batch of dentries
> and it may cause dir/files duplication for NFS readdirplus which
> depends on ctx->pos to fill dir correctly. So update ctx->pos for
> every emitted dirent in erofs_fill_dentries to fix it.
>
> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> ---
> fs/erofs/dir.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 18e59821c597..3015974fe2ff 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -22,11 +22,12 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
> }
>
> static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> - void *dentry_blk, unsigned int *ofs,
> + void *dentry_blk, void *dentry_begin,
> unsigned int nameoff, unsigned int maxsize)
> {
> - struct erofs_dirent *de = dentry_blk + *ofs;
> + struct erofs_dirent *de = dentry_begin;
> const struct erofs_dirent *end = dentry_blk + nameoff;
> + loff_t begin_pos = ctx->pos;
>
> while (de < end) {
> const char *de_name;
> @@ -59,9 +60,9 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> /* stopped by some reason */
> return 1;
> ++de;
> - *ofs += sizeof(struct erofs_dirent);
> + ctx->pos += sizeof(struct erofs_dirent);
> }
> - *ofs = maxsize;
> + ctx->pos = begin_pos + maxsize;
> return 0;
> }
>
> @@ -110,11 +111,9 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> goto skip_this;
> }
>
> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> + err = erofs_fill_dentries(dir, ctx, de, de + ofs,
> nameoff, maxsize);
This will break the calculation, since de is a pointer of erofs_dirent
rather than byte-based.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] erofs: update ctx->pos for every emitted dirent
@ 2022-05-28 12:56 ` Gao Xiang
0 siblings, 0 replies; 13+ messages in thread
From: Gao Xiang @ 2022-05-28 12:56 UTC (permalink / raw)
To: Hongnan Li; +Cc: linux-erofs, linux-kernel
Hi Hongnan,
On Fri, May 27, 2022 at 03:25:36PM +0800, Hongnan Li wrote:
> erofs_readdir update ctx->pos after filling a batch of dentries
> and it may cause dir/files duplication for NFS readdirplus which
> depends on ctx->pos to fill dir correctly. So update ctx->pos for
> every emitted dirent in erofs_fill_dentries to fix it.
>
> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> ---
> fs/erofs/dir.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 18e59821c597..3015974fe2ff 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -22,11 +22,12 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
> }
>
> static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> - void *dentry_blk, unsigned int *ofs,
> + void *dentry_blk, void *dentry_begin,
> unsigned int nameoff, unsigned int maxsize)
> {
> - struct erofs_dirent *de = dentry_blk + *ofs;
> + struct erofs_dirent *de = dentry_begin;
> const struct erofs_dirent *end = dentry_blk + nameoff;
> + loff_t begin_pos = ctx->pos;
>
> while (de < end) {
> const char *de_name;
> @@ -59,9 +60,9 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> /* stopped by some reason */
> return 1;
> ++de;
> - *ofs += sizeof(struct erofs_dirent);
> + ctx->pos += sizeof(struct erofs_dirent);
> }
> - *ofs = maxsize;
> + ctx->pos = begin_pos + maxsize;
> return 0;
> }
>
> @@ -110,11 +111,9 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> goto skip_this;
> }
>
> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> + err = erofs_fill_dentries(dir, ctx, de, de + ofs,
> nameoff, maxsize);
This will break the calculation, since de is a pointer of erofs_dirent
rather than byte-based.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] erofs: update ctx->pos for every emitted dirent
2022-05-27 7:25 [PATCH] erofs: update ctx->pos for every emitted dirent Hongnan Li
2022-05-28 12:56 ` Gao Xiang
@ 2022-05-29 6:26 ` Chao Yu
2022-06-09 3:40 ` [PATCH v2] " Hongnan Li
2022-06-29 8:15 ` [PATCH v3] " Hongnan Li
3 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2022-05-29 6:26 UTC (permalink / raw)
To: Hongnan Li, linux-erofs, xiang; +Cc: linux-kernel
On 2022/5/27 15:25, Hongnan Li wrote:
> erofs_readdir update ctx->pos after filling a batch of dentries
> and it may cause dir/files duplication for NFS readdirplus which
> depends on ctx->pos to fill dir correctly. So update ctx->pos for
> every emitted dirent in erofs_fill_dentries to fix it.
>
> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> ---
> fs/erofs/dir.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 18e59821c597..3015974fe2ff 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -22,11 +22,12 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
> }
>
> static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> - void *dentry_blk, unsigned int *ofs,
> + void *dentry_blk, void *dentry_begin,
> unsigned int nameoff, unsigned int maxsize)
> {
> - struct erofs_dirent *de = dentry_blk + *ofs;
> + struct erofs_dirent *de = dentry_begin;
> const struct erofs_dirent *end = dentry_blk + nameoff;
> + loff_t begin_pos = ctx->pos;
>
> while (de < end) {
> const char *de_name;
> @@ -59,9 +60,9 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> /* stopped by some reason */
> return 1;
> ++de;
> - *ofs += sizeof(struct erofs_dirent);
> + ctx->pos += sizeof(struct erofs_dirent);
> }
> - *ofs = maxsize;
> + ctx->pos = begin_pos + maxsize;
> return 0;
> }
>
> @@ -110,11 +111,9 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> goto skip_this;
> }
>
> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> + err = erofs_fill_dentries(dir, ctx, de, de + ofs,
> nameoff, maxsize);
> skip_this:
I guess there are two paths may be affected in erofs_readdir():
- for EFSCORRUPTED case, how about avoiding "goto skip_this" since we
can just break there.
- for initial case, do we need to update ctx->pos as well?
Thanks,
> - ctx->pos = blknr_to_addr(i) + ofs;
> -
> if (err)
> break;
> ++i;
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2] erofs: update ctx->pos for every emitted dirent
2022-05-27 7:25 [PATCH] erofs: update ctx->pos for every emitted dirent Hongnan Li
2022-05-28 12:56 ` Gao Xiang
2022-05-29 6:26 ` Chao Yu
@ 2022-06-09 3:40 ` Hongnan Li
2022-06-19 0:19 ` Chao Yu
2022-06-29 8:15 ` [PATCH v3] " Hongnan Li
3 siblings, 1 reply; 13+ messages in thread
From: Hongnan Li @ 2022-06-09 3:40 UTC (permalink / raw)
To: linux-erofs, xiang, chao; +Cc: linux-kernel
erofs_readdir update ctx->pos after filling a batch of dentries
and it may cause dir/files duplication for NFS readdirplus which
depends on ctx->pos to fill dir correctly. So update ctx->pos for
every emitted dirent in erofs_fill_dentries to fix it.
Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
---
fs/erofs/dir.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index 18e59821c597..94ef5287237a 100644
--- a/fs/erofs/dir.c
+++ b/fs/erofs/dir.c
@@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
}
static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
- void *dentry_blk, unsigned int *ofs,
+ void *dentry_blk, struct erofs_dirent *de,
unsigned int nameoff, unsigned int maxsize)
{
- struct erofs_dirent *de = dentry_blk + *ofs;
const struct erofs_dirent *end = dentry_blk + nameoff;
while (de < end) {
@@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
/* stopped by some reason */
return 1;
++de;
- *ofs += sizeof(struct erofs_dirent);
+ ctx->pos += sizeof(struct erofs_dirent);
}
- *ofs = maxsize;
return 0;
}
@@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
"invalid de[0].nameoff %u @ nid %llu",
nameoff, EROFS_I(dir)->nid);
err = -EFSCORRUPTED;
- goto skip_this;
+ break;
}
maxsize = min_t(unsigned int,
@@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
initial = false;
ofs = roundup(ofs, sizeof(struct erofs_dirent));
- if (ofs >= nameoff)
+ if (ofs >= nameoff) {
+ ctx->pos = blknr_to_addr(i) + ofs;
goto skip_this;
+ }
}
- err = erofs_fill_dentries(dir, ctx, de, &ofs,
- nameoff, maxsize);
-skip_this:
ctx->pos = blknr_to_addr(i) + ofs;
-
+ err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
+ nameoff, maxsize);
if (err)
break;
+ ctx->pos = blknr_to_addr(i) + maxsize;
+skip_this:
++i;
ofs = 0;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
2022-06-09 3:40 ` [PATCH v2] " Hongnan Li
@ 2022-06-19 0:19 ` Chao Yu
2022-06-20 9:37 ` hongnanLi
0 siblings, 1 reply; 13+ messages in thread
From: Chao Yu @ 2022-06-19 0:19 UTC (permalink / raw)
To: Hongnan Li, linux-erofs, xiang; +Cc: linux-kernel
On 2022/6/9 11:40, Hongnan Li wrote:
> erofs_readdir update ctx->pos after filling a batch of dentries
> and it may cause dir/files duplication for NFS readdirplus which
> depends on ctx->pos to fill dir correctly. So update ctx->pos for
> every emitted dirent in erofs_fill_dentries to fix it.
>
> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> ---
> fs/erofs/dir.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 18e59821c597..94ef5287237a 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
> }
>
> static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> - void *dentry_blk, unsigned int *ofs,
> + void *dentry_blk, struct erofs_dirent *de,
> unsigned int nameoff, unsigned int maxsize)
> {
> - struct erofs_dirent *de = dentry_blk + *ofs;
> const struct erofs_dirent *end = dentry_blk + nameoff;
>
> while (de < end) {
> @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> /* stopped by some reason */
> return 1;
> ++de;
> - *ofs += sizeof(struct erofs_dirent);
> + ctx->pos += sizeof(struct erofs_dirent);
> }
> - *ofs = maxsize;
> return 0;
> }
>
> @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> "invalid de[0].nameoff %u @ nid %llu",
> nameoff, EROFS_I(dir)->nid);
> err = -EFSCORRUPTED;
> - goto skip_this;
> + break;
> }
>
> maxsize = min_t(unsigned int,
> @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> initial = false;
>
> ofs = roundup(ofs, sizeof(struct erofs_dirent));
> - if (ofs >= nameoff)
> + if (ofs >= nameoff) {
> + ctx->pos = blknr_to_addr(i) + ofs;
> goto skip_this;
> + }
> }
>
> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> - nameoff, maxsize);
> -skip_this:
> ctx->pos = blknr_to_addr(i) + ofs;
Why updating ctx->pos before erofs_fill_dentries()?
Thanks,
> -
> + err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
> + nameoff, maxsize);
> if (err)
> break;
> + ctx->pos = blknr_to_addr(i) + maxsize;
> +skip_this:
> ++i;
> ofs = 0;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
2022-06-19 0:19 ` Chao Yu
@ 2022-06-20 9:37 ` hongnanLi
2022-06-20 12:19 ` Gao Xiang
0 siblings, 1 reply; 13+ messages in thread
From: hongnanLi @ 2022-06-20 9:37 UTC (permalink / raw)
To: Chao Yu, linux-erofs, xiang; +Cc: linux-kernel
on 2022/6/19 8:19, Chao Yu wrote:
> On 2022/6/9 11:40, Hongnan Li wrote:
>> erofs_readdir update ctx->pos after filling a batch of dentries
>> and it may cause dir/files duplication for NFS readdirplus which
>> depends on ctx->pos to fill dir correctly. So update ctx->pos for
>> every emitted dirent in erofs_fill_dentries to fix it.
>>
>> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
>> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
>> ---
>> fs/erofs/dir.c | 20 ++++++++++----------
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>> index 18e59821c597..94ef5287237a 100644
>> --- a/fs/erofs/dir.c
>> +++ b/fs/erofs/dir.c
>> @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char d_type,
>> const char *de_name,
>> }
>> static int erofs_fill_dentries(struct inode *dir, struct dir_context
>> *ctx,
>> - void *dentry_blk, unsigned int *ofs,
>> + void *dentry_blk, struct erofs_dirent *de,
>> unsigned int nameoff, unsigned int maxsize)
>> {
>> - struct erofs_dirent *de = dentry_blk + *ofs;
>> const struct erofs_dirent *end = dentry_blk + nameoff;
>> while (de < end) {
>> @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir,
>> struct dir_context *ctx,
>> /* stopped by some reason */
>> return 1;
>> ++de;
>> - *ofs += sizeof(struct erofs_dirent);
>> + ctx->pos += sizeof(struct erofs_dirent);
>> }
>> - *ofs = maxsize;
>> return 0;
>> }
>> @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct
>> dir_context *ctx)
>> "invalid de[0].nameoff %u @ nid %llu",
>> nameoff, EROFS_I(dir)->nid);
>> err = -EFSCORRUPTED;
>> - goto skip_this;
>> + break;
>> }
>> maxsize = min_t(unsigned int,
>> @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f, struct
>> dir_context *ctx)
>> initial = false;
>> ofs = roundup(ofs, sizeof(struct erofs_dirent));
>> - if (ofs >= nameoff)
>> + if (ofs >= nameoff) {
>> + ctx->pos = blknr_to_addr(i) + ofs;
>> goto skip_this;
>> + }
>> }
>> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
>> - nameoff, maxsize);
>> -skip_this:
>> ctx->pos = blknr_to_addr(i) + ofs;
>
> Why updating ctx->pos before erofs_fill_dentries()?
>
> Thanks,
It’s to ensure the ctx->pos is correct and up to date in
erofs_fill_dentries() so that we can update ctx->pos instead of ofs for
every emitted dirent.
>
>> -
>> + err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
>> + nameoff, maxsize);
>> if (err)
>> break;
>> + ctx->pos = blknr_to_addr(i) + maxsize;
>> +skip_this:
>> ++i;
>> ofs = 0;
>> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
2022-06-20 9:37 ` hongnanLi
@ 2022-06-20 12:19 ` Gao Xiang
0 siblings, 0 replies; 13+ messages in thread
From: Gao Xiang @ 2022-06-20 12:19 UTC (permalink / raw)
To: hongnanLi; +Cc: Chao Yu, linux-erofs, xiang, linux-kernel
Hi Hongnan,
On Mon, Jun 20, 2022 at 05:37:07PM +0800, hongnanLi wrote:
> on 2022/6/19 8:19, Chao Yu wrote:
> > On 2022/6/9 11:40, Hongnan Li wrote:
> > > erofs_readdir update ctx->pos after filling a batch of dentries
> > > and it may cause dir/files duplication for NFS readdirplus which
> > > depends on ctx->pos to fill dir correctly. So update ctx->pos for
> > > every emitted dirent in erofs_fill_dentries to fix it.
> > >
> > > Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> > > Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> > > ---
> > > fs/erofs/dir.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> > > index 18e59821c597..94ef5287237a 100644
> > > --- a/fs/erofs/dir.c
> > > +++ b/fs/erofs/dir.c
> > > @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char
> > > d_type, const char *de_name,
> > > }
> > > static int erofs_fill_dentries(struct inode *dir, struct
> > > dir_context *ctx,
> > > - void *dentry_blk, unsigned int *ofs,
> > > + void *dentry_blk, struct erofs_dirent *de,
> > > unsigned int nameoff, unsigned int maxsize)
> > > {
> > > - struct erofs_dirent *de = dentry_blk + *ofs;
> > > const struct erofs_dirent *end = dentry_blk + nameoff;
> > > while (de < end) {
> > > @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir,
> > > struct dir_context *ctx,
> > > /* stopped by some reason */
> > > return 1;
> > > ++de;
> > > - *ofs += sizeof(struct erofs_dirent);
> > > + ctx->pos += sizeof(struct erofs_dirent);
> > > }
> > > - *ofs = maxsize;
> > > return 0;
> > > }
> > > @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct
> > > dir_context *ctx)
> > > "invalid de[0].nameoff %u @ nid %llu",
> > > nameoff, EROFS_I(dir)->nid);
> > > err = -EFSCORRUPTED;
> > > - goto skip_this;
> > > + break;
> > > }
> > > maxsize = min_t(unsigned int,
> > > @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f,
> > > struct dir_context *ctx)
> > > initial = false;
> > > ofs = roundup(ofs, sizeof(struct erofs_dirent));
> > > - if (ofs >= nameoff)
> > > + if (ofs >= nameoff) {
> > > + ctx->pos = blknr_to_addr(i) + ofs;
> > > goto skip_this;
> > > + }
> > > }
> > > - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> > > - nameoff, maxsize);
> > > -skip_this:
> > > ctx->pos = blknr_to_addr(i) + ofs;
> >
> > Why updating ctx->pos before erofs_fill_dentries()?
> >
> > Thanks,
>
> It’s to ensure the ctx->pos is correct and up to date in
> erofs_fill_dentries() so that we can update ctx->pos instead of ofs for
> every emitted dirent.
>
How about this, since blknr_to_addr(i) + maxsize should be the start of
the next dir block.
if (initial) {
ofs = roundup(ofs, sizeof(struct erofs_dirent));
ctx->pos = blknr_to_addr(i) + ofs;
if (ofs >= nameoff)
goto skip_this;
}
err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
nameoff, maxsize);
if (err)
break;
ctx->pos = blknr_to_addr(i) + maxsize;
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
@ 2022-06-20 12:19 ` Gao Xiang
0 siblings, 0 replies; 13+ messages in thread
From: Gao Xiang @ 2022-06-20 12:19 UTC (permalink / raw)
To: hongnanLi; +Cc: linux-erofs, linux-kernel
Hi Hongnan,
On Mon, Jun 20, 2022 at 05:37:07PM +0800, hongnanLi wrote:
> on 2022/6/19 8:19, Chao Yu wrote:
> > On 2022/6/9 11:40, Hongnan Li wrote:
> > > erofs_readdir update ctx->pos after filling a batch of dentries
> > > and it may cause dir/files duplication for NFS readdirplus which
> > > depends on ctx->pos to fill dir correctly. So update ctx->pos for
> > > every emitted dirent in erofs_fill_dentries to fix it.
> > >
> > > Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> > > Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> > > ---
> > > fs/erofs/dir.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> > > index 18e59821c597..94ef5287237a 100644
> > > --- a/fs/erofs/dir.c
> > > +++ b/fs/erofs/dir.c
> > > @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char
> > > d_type, const char *de_name,
> > > }
> > > static int erofs_fill_dentries(struct inode *dir, struct
> > > dir_context *ctx,
> > > - void *dentry_blk, unsigned int *ofs,
> > > + void *dentry_blk, struct erofs_dirent *de,
> > > unsigned int nameoff, unsigned int maxsize)
> > > {
> > > - struct erofs_dirent *de = dentry_blk + *ofs;
> > > const struct erofs_dirent *end = dentry_blk + nameoff;
> > > while (de < end) {
> > > @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir,
> > > struct dir_context *ctx,
> > > /* stopped by some reason */
> > > return 1;
> > > ++de;
> > > - *ofs += sizeof(struct erofs_dirent);
> > > + ctx->pos += sizeof(struct erofs_dirent);
> > > }
> > > - *ofs = maxsize;
> > > return 0;
> > > }
> > > @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct
> > > dir_context *ctx)
> > > "invalid de[0].nameoff %u @ nid %llu",
> > > nameoff, EROFS_I(dir)->nid);
> > > err = -EFSCORRUPTED;
> > > - goto skip_this;
> > > + break;
> > > }
> > > maxsize = min_t(unsigned int,
> > > @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f,
> > > struct dir_context *ctx)
> > > initial = false;
> > > ofs = roundup(ofs, sizeof(struct erofs_dirent));
> > > - if (ofs >= nameoff)
> > > + if (ofs >= nameoff) {
> > > + ctx->pos = blknr_to_addr(i) + ofs;
> > > goto skip_this;
> > > + }
> > > }
> > > - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> > > - nameoff, maxsize);
> > > -skip_this:
> > > ctx->pos = blknr_to_addr(i) + ofs;
> >
> > Why updating ctx->pos before erofs_fill_dentries()?
> >
> > Thanks,
>
> It’s to ensure the ctx->pos is correct and up to date in
> erofs_fill_dentries() so that we can update ctx->pos instead of ofs for
> every emitted dirent.
>
How about this, since blknr_to_addr(i) + maxsize should be the start of
the next dir block.
if (initial) {
ofs = roundup(ofs, sizeof(struct erofs_dirent));
ctx->pos = blknr_to_addr(i) + ofs;
if (ofs >= nameoff)
goto skip_this;
}
err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
nameoff, maxsize);
if (err)
break;
ctx->pos = blknr_to_addr(i) + maxsize;
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
2022-06-20 12:19 ` Gao Xiang
@ 2022-06-24 9:31 ` hongnanLi
-1 siblings, 0 replies; 13+ messages in thread
From: hongnanLi @ 2022-06-24 9:31 UTC (permalink / raw)
To: Gao Xiang; +Cc: Chao Yu, linux-erofs, xiang, linux-kernel
Hi Gao Xiang,
on 2022/6/20 下午8:19, Gao Xiang wrote:
> Hi Hongnan,
>
> On Mon, Jun 20, 2022 at 05:37:07PM +0800, hongnanLi wrote:
>> on 2022/6/19 8:19, Chao Yu wrote:
>>> On 2022/6/9 11:40, Hongnan Li wrote:
>>>> erofs_readdir update ctx->pos after filling a batch of dentries
>>>> and it may cause dir/files duplication for NFS readdirplus which
>>>> depends on ctx->pos to fill dir correctly. So update ctx->pos for
>>>> every emitted dirent in erofs_fill_dentries to fix it.
>>>>
>>>> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
>>>> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
>>>> ---
>>>> fs/erofs/dir.c | 20 ++++++++++----------
>>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>>>> index 18e59821c597..94ef5287237a 100644
>>>> --- a/fs/erofs/dir.c
>>>> +++ b/fs/erofs/dir.c
>>>> @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char
>>>> d_type, const char *de_name,
>>>> }
>>>> static int erofs_fill_dentries(struct inode *dir, struct
>>>> dir_context *ctx,
>>>> - void *dentry_blk, unsigned int *ofs,
>>>> + void *dentry_blk, struct erofs_dirent *de,
>>>> unsigned int nameoff, unsigned int maxsize)
>>>> {
>>>> - struct erofs_dirent *de = dentry_blk + *ofs;
>>>> const struct erofs_dirent *end = dentry_blk + nameoff;
>>>> while (de < end) {
>>>> @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir,
>>>> struct dir_context *ctx,
>>>> /* stopped by some reason */
>>>> return 1;
>>>> ++de;
>>>> - *ofs += sizeof(struct erofs_dirent);
>>>> + ctx->pos += sizeof(struct erofs_dirent);
>>>> }
>>>> - *ofs = maxsize;
>>>> return 0;
>>>> }
>>>> @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct
>>>> dir_context *ctx)
>>>> "invalid de[0].nameoff %u @ nid %llu",
>>>> nameoff, EROFS_I(dir)->nid);
>>>> err = -EFSCORRUPTED;
>>>> - goto skip_this;
>>>> + break;
>>>> }
>>>> maxsize = min_t(unsigned int,
>>>> @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f,
>>>> struct dir_context *ctx)
>>>> initial = false;
>>>> ofs = roundup(ofs, sizeof(struct erofs_dirent));
>>>> - if (ofs >= nameoff)
>>>> + if (ofs >= nameoff) {
>>>> + ctx->pos = blknr_to_addr(i) + ofs;
>>>> goto skip_this;
>>>> + }
>>>> }
>>>> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
>>>> - nameoff, maxsize);
>>>> -skip_this:
>>>> ctx->pos = blknr_to_addr(i) + ofs;
>>>
>>> Why updating ctx->pos before erofs_fill_dentries()?
>>>
>>> Thanks,
>>
>> It’s to ensure the ctx->pos is correct and up to date in
>> erofs_fill_dentries() so that we can update ctx->pos instead of ofs for
>> every emitted dirent.
>>
>
> How about this, since blknr_to_addr(i) + maxsize should be the start of
> the next dir block.
>
> if (initial) {
> ofs = roundup(ofs, sizeof(struct erofs_dirent));
> ctx->pos = blknr_to_addr(i) + ofs;
> if (ofs >= nameoff)
> goto skip_this;
> }
> err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
> nameoff, maxsize);
> if (err)
> break;
> ctx->pos = blknr_to_addr(i) + maxsize;
>
Thanks for your suggestion. It looks good and works well in my test. I
will send PATCH v3 later if everything else is okay.
Thanks,
Hongnan Li
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] erofs: update ctx->pos for every emitted dirent
@ 2022-06-24 9:31 ` hongnanLi
0 siblings, 0 replies; 13+ messages in thread
From: hongnanLi @ 2022-06-24 9:31 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-erofs, linux-kernel
Hi Gao Xiang,
on 2022/6/20 下午8:19, Gao Xiang wrote:
> Hi Hongnan,
>
> On Mon, Jun 20, 2022 at 05:37:07PM +0800, hongnanLi wrote:
>> on 2022/6/19 8:19, Chao Yu wrote:
>>> On 2022/6/9 11:40, Hongnan Li wrote:
>>>> erofs_readdir update ctx->pos after filling a batch of dentries
>>>> and it may cause dir/files duplication for NFS readdirplus which
>>>> depends on ctx->pos to fill dir correctly. So update ctx->pos for
>>>> every emitted dirent in erofs_fill_dentries to fix it.
>>>>
>>>> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
>>>> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
>>>> ---
>>>> fs/erofs/dir.c | 20 ++++++++++----------
>>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
>>>> index 18e59821c597..94ef5287237a 100644
>>>> --- a/fs/erofs/dir.c
>>>> +++ b/fs/erofs/dir.c
>>>> @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char
>>>> d_type, const char *de_name,
>>>> }
>>>> static int erofs_fill_dentries(struct inode *dir, struct
>>>> dir_context *ctx,
>>>> - void *dentry_blk, unsigned int *ofs,
>>>> + void *dentry_blk, struct erofs_dirent *de,
>>>> unsigned int nameoff, unsigned int maxsize)
>>>> {
>>>> - struct erofs_dirent *de = dentry_blk + *ofs;
>>>> const struct erofs_dirent *end = dentry_blk + nameoff;
>>>> while (de < end) {
>>>> @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir,
>>>> struct dir_context *ctx,
>>>> /* stopped by some reason */
>>>> return 1;
>>>> ++de;
>>>> - *ofs += sizeof(struct erofs_dirent);
>>>> + ctx->pos += sizeof(struct erofs_dirent);
>>>> }
>>>> - *ofs = maxsize;
>>>> return 0;
>>>> }
>>>> @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct
>>>> dir_context *ctx)
>>>> "invalid de[0].nameoff %u @ nid %llu",
>>>> nameoff, EROFS_I(dir)->nid);
>>>> err = -EFSCORRUPTED;
>>>> - goto skip_this;
>>>> + break;
>>>> }
>>>> maxsize = min_t(unsigned int,
>>>> @@ -106,17 +104,19 @@ static int erofs_readdir(struct file *f,
>>>> struct dir_context *ctx)
>>>> initial = false;
>>>> ofs = roundup(ofs, sizeof(struct erofs_dirent));
>>>> - if (ofs >= nameoff)
>>>> + if (ofs >= nameoff) {
>>>> + ctx->pos = blknr_to_addr(i) + ofs;
>>>> goto skip_this;
>>>> + }
>>>> }
>>>> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
>>>> - nameoff, maxsize);
>>>> -skip_this:
>>>> ctx->pos = blknr_to_addr(i) + ofs;
>>>
>>> Why updating ctx->pos before erofs_fill_dentries()?
>>>
>>> Thanks,
>>
>> It’s to ensure the ctx->pos is correct and up to date in
>> erofs_fill_dentries() so that we can update ctx->pos instead of ofs for
>> every emitted dirent.
>>
>
> How about this, since blknr_to_addr(i) + maxsize should be the start of
> the next dir block.
>
> if (initial) {
> ofs = roundup(ofs, sizeof(struct erofs_dirent));
> ctx->pos = blknr_to_addr(i) + ofs;
> if (ofs >= nameoff)
> goto skip_this;
> }
> err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
> nameoff, maxsize);
> if (err)
> break;
> ctx->pos = blknr_to_addr(i) + maxsize;
>
Thanks for your suggestion. It looks good and works well in my test. I
will send PATCH v3 later if everything else is okay.
Thanks,
Hongnan Li
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] erofs: update ctx->pos for every emitted dirent
2022-05-27 7:25 [PATCH] erofs: update ctx->pos for every emitted dirent Hongnan Li
` (2 preceding siblings ...)
2022-06-09 3:40 ` [PATCH v2] " Hongnan Li
@ 2022-06-29 8:15 ` Hongnan Li
2022-07-22 8:16 ` JeffleXu
3 siblings, 1 reply; 13+ messages in thread
From: Hongnan Li @ 2022-06-29 8:15 UTC (permalink / raw)
To: linux-erofs, xiang, chao; +Cc: linux-kernel
erofs_readdir update ctx->pos after filling a batch of dentries
and it may cause dir/files duplication for NFS readdirplus which
depends on ctx->pos to fill dir correctly. So update ctx->pos for
every emitted dirent in erofs_fill_dentries to fix it.
Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
---
fs/erofs/dir.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
index 18e59821c597..6fc325052853 100644
--- a/fs/erofs/dir.c
+++ b/fs/erofs/dir.c
@@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
}
static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
- void *dentry_blk, unsigned int *ofs,
+ void *dentry_blk, struct erofs_dirent *de,
unsigned int nameoff, unsigned int maxsize)
{
- struct erofs_dirent *de = dentry_blk + *ofs;
const struct erofs_dirent *end = dentry_blk + nameoff;
while (de < end) {
@@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
/* stopped by some reason */
return 1;
++de;
- *ofs += sizeof(struct erofs_dirent);
+ ctx->pos += sizeof(struct erofs_dirent);
}
- *ofs = maxsize;
return 0;
}
@@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
"invalid de[0].nameoff %u @ nid %llu",
nameoff, EROFS_I(dir)->nid);
err = -EFSCORRUPTED;
- goto skip_this;
+ break;
}
maxsize = min_t(unsigned int,
@@ -106,17 +104,17 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
initial = false;
ofs = roundup(ofs, sizeof(struct erofs_dirent));
+ ctx->pos = blknr_to_addr(i) + ofs;
if (ofs >= nameoff)
goto skip_this;
}
- err = erofs_fill_dentries(dir, ctx, de, &ofs,
+ err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
nameoff, maxsize);
-skip_this:
- ctx->pos = blknr_to_addr(i) + ofs;
-
if (err)
break;
+ ctx->pos = blknr_to_addr(i) + maxsize;
+skip_this:
++i;
ofs = 0;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3] erofs: update ctx->pos for every emitted dirent
2022-06-29 8:15 ` [PATCH v3] " Hongnan Li
@ 2022-07-22 8:16 ` JeffleXu
0 siblings, 0 replies; 13+ messages in thread
From: JeffleXu @ 2022-07-22 8:16 UTC (permalink / raw)
To: Hongnan Li, linux-erofs, xiang, chao; +Cc: linux-kernel
The patch itself looks good to me.
On 6/29/22 4:15 PM, Hongnan Li wrote:
> erofs_readdir update ctx->pos after filling a batch of dentries
> and it may cause dir/files duplication for NFS readdirplus which
> depends on ctx->pos to fill dir correctly. So update ctx->pos for
> every emitted dirent in erofs_fill_dentries to fix it.
>
> Fixes: 3e917cc305c6 ("erofs: make filesystem exportable")
> Signed-off-by: Hongnan Li <hongnan.li@linux.alibaba.com>
> ---
> fs/erofs/dir.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/fs/erofs/dir.c b/fs/erofs/dir.c
> index 18e59821c597..6fc325052853 100644
> --- a/fs/erofs/dir.c
> +++ b/fs/erofs/dir.c
> @@ -22,10 +22,9 @@ static void debug_one_dentry(unsigned char d_type, const char *de_name,
> }
>
> static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> - void *dentry_blk, unsigned int *ofs,
> + void *dentry_blk, struct erofs_dirent *de,
> unsigned int nameoff, unsigned int maxsize)
> {
> - struct erofs_dirent *de = dentry_blk + *ofs;
> const struct erofs_dirent *end = dentry_blk + nameoff;
>
> while (de < end) {
> @@ -59,9 +58,8 @@ static int erofs_fill_dentries(struct inode *dir, struct dir_context *ctx,
> /* stopped by some reason */
> return 1;
> ++de;
> - *ofs += sizeof(struct erofs_dirent);
> + ctx->pos += sizeof(struct erofs_dirent);
> }
> - *ofs = maxsize;
> return 0;
> }
>
> @@ -95,7 +93,7 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> "invalid de[0].nameoff %u @ nid %llu",
> nameoff, EROFS_I(dir)->nid);
> err = -EFSCORRUPTED;
> - goto skip_this;
> + break;
> }
>
> maxsize = min_t(unsigned int,
> @@ -106,17 +104,17 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> initial = false;
>
> ofs = roundup(ofs, sizeof(struct erofs_dirent));
> + ctx->pos = blknr_to_addr(i) + ofs;
> if (ofs >= nameoff)
> goto skip_this;
Besides, I thinks there's another issue with erofs_readdir() here
(though unrelated to the issue this patch wants to fix).
We need to update ctx->pos correctly if the initial file position has
exceeded nameoff. ctx->pos needs to be updated to the end of
EROFS_BLKSIZ or directory's i_size, surpassing the remaining name string
in the current EROFS block.
> }
>
> - err = erofs_fill_dentries(dir, ctx, de, &ofs,
> + err = erofs_fill_dentries(dir, ctx, de, (void *)de + ofs,
> nameoff, maxsize);
> -skip_this:
> - ctx->pos = blknr_to_addr(i) + ofs;
> -
> if (err)
> break;
> + ctx->pos = blknr_to_addr(i) + maxsize;
It's quite easy to fix the above issue. We only need to move this line
beneath skip_this label.
> +skip_this:> ++i;
> ofs = 0;
> }
like:
skip_this:
ctx->pos = blknr_to_addr(i) + maxsize;
++i;
ofs = 0;
Thus we'd better fold this simple fix into this patch.
--
Thanks,
Jeffle
^ permalink raw reply [flat|nested] 13+ messages in thread