fuse returns ENOENT to openat() for symlink probabilistically

fuse returns ENOENT to openat() for symlink probabilistically

[胡玮文]

Hi all,

I'm working on setting up CI service to run tests automatically. Now I have got
tests with kernel mount succeeded. But some tests with fuse fails
probabilistically. Here are my discoveries:

* if I run fssum in tests/src from experimental-tests branch multiple times, it
returns different checksums for the same image and same erofsfuse process.

* if I run "diff -r" on the source and the mounted directories, all file
content matches. but sometimes, diff reports "diff:
test-mount/lib/.libs/liberofs.la: No such file or directory". This file is a
symlink to "../liberofs.la". Then I use strace to confirm that openat() system
call to this path returned ENOENT incorrectly. strace outputs:

openat(AT_FDCWD, "test-mount/lib/.libs/liberofs.la", O_RDONLY) = -1 ENOENT (No such file or directory)

* However, If I just do "cat test-mount/lib/.libs/liberofs.la" several hundreds
of times, I cannot trigger this issue.

* I can reproduce this on both compressed and uncompressed images.

There seems a race condition, but I cannot figure it out. I'm not familiar with
fuse. But I would like to debug further if someone can provide me any advice or
guidance.

Thanks,
Hu Weiwen

Re: fuse returns ENOENT to openat() for symlink probabilistically

[Gao Xiang]

Hi Weiwen,

(add Jianan in case that he has some interest as well)

On Thu, Jan 21, 2021 at 06:12:33PM +0800, 胡玮文 wrote:
> Hi all,
> 
> I'm working on setting up CI service to run tests automatically. Now I have got
> tests with kernel mount succeeded. But some tests with fuse fails
> probabilistically. Here are my discoveries:
>

If you have some interest, it would be better to add '-d' to
erofsfuse to print the debug message when reproduing that,
if that is not enough we might need to add more debug messages
(just using fprintf is enough since it's MT-safe.)

I will checked this case this weekend as well.

> * if I run fssum in tests/src from experimental-tests branch multiple times, it
> returns different checksums for the same image and same erofsfuse process.
> 
> * if I run "diff -r" on the source and the mounted directories, all file
> content matches. but sometimes, diff reports "diff:
> test-mount/lib/.libs/liberofs.la: No such file or directory". This file is a
> symlink to "../liberofs.la". Then I use strace to confirm that openat() system
> call to this path returned ENOENT incorrectly. strace outputs:
> 
> openat(AT_FDCWD, "test-mount/lib/.libs/liberofs.la", O_RDONLY) = -1 ENOENT (No such file or directory)
> 
> * However, If I just do "cat test-mount/lib/.libs/liberofs.la" several hundreds
> of times, I cannot trigger this issue.
> 
> * I can reproduce this on both compressed and uncompressed images.
> 
> There seems a race condition, but I cannot figure it out. I'm not familiar with
> fuse. But I would like to debug further if someone can provide me any advice or
> guidance.

Not sure if this is a race condition (or not), since currently erofsfuse
itself is implemented statelessly (so no need to add more MT-safe protect
in principle). I'm afraid that could be something wrong somewhere (which
seems somewhat as stale VFS negative dentries, yet not sure why it behaves
as this...)

Thanks,
Gao Xiang

> 
> Thanks,
> Hu Weiwen
> 

[PATCH] erofs-utils: fuse: fix random readlink error

[Hu Weiwen]

readlink should fill a **null terminated** string in buffer.

Also, read should return number of bytes remaining on EOF.

Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
---
 fuse/main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fuse/main.c b/fuse/main.c
index c162912..bc1e496 100644
--- a/fuse/main.c
+++ b/fuse/main.c
@@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
 	if (ret)
 		return ret;
 
+	if (offset >= vi.i_size)
+		return 0;
+
+	if (offset + size > vi.i_size)
+		size = vi.i_size - offset;
+
 	ret = erofs_pread(&vi, buffer, size, offset);
 	if (ret)
 		return ret;
@@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
 
 static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
 {
-	int ret = erofsfuse_read(path, buffer, size, 0, NULL);
+	int ret;
+	size_t path_len;
+
+	erofs_dbg("path:%s size=%zd", path, size);
+	ret = erofsfuse_read(path, buffer, size, 0, NULL);
 
 	if (ret < 0)
 		return ret;
+	path_len = min(size - 1, (size_t)ret);
+	buffer[path_len] = '\0';
 	return 0;
 }
 
-- 
2.30.0

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[Gao Xiang]

Hi Weiwen,

On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> readlink should fill a **null terminated** string in buffer.
> 
> Also, read should return number of bytes remaining on EOF.
> 
> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>

Thanks for catching this!

> ---
>  fuse/main.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fuse/main.c b/fuse/main.c
> index c162912..bc1e496 100644
> --- a/fuse/main.c
> +++ b/fuse/main.c
> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
>  	if (ret)
>  		return ret;
>  
> +	if (offset >= vi.i_size)
> +		return 0;
> +
> +	if (offset + size > vi.i_size)
> +		size = vi.i_size - offset;
> +

It would be better to call erofs_pread() with the original offset
and size (also I think there is some missing memset(0) for
!EROFS_MAP_MAPPED), and fix up the return value to the needed value.

Thanks,
Gao Xiang

>  	ret = erofs_pread(&vi, buffer, size, offset);
>  	if (ret)
>  		return ret;
> @@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
>  
>  static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
>  {
> -	int ret = erofsfuse_read(path, buffer, size, 0, NULL);
> +	int ret;
> +	size_t path_len;
> +
> +	erofs_dbg("path:%s size=%zd", path, size);
> +	ret = erofsfuse_read(path, buffer, size, 0, NULL);
>  
>  	if (ret < 0)
>  		return ret;
> +	path_len = min(size - 1, (size_t)ret);
> +	buffer[path_len] = '\0';
>  	return 0;
>  }
>  
> -- 
> 2.30.0
> 

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[胡玮文]

Hi Xiang,

> 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@redhat.com> 写道：
> 
> Hi Weiwen,
> 
>> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
>> readlink should fill a **null terminated** string in buffer.
>> 
>> Also, read should return number of bytes remaining on EOF.
>> 
>> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> 
> Thanks for catching this!
> 
>> ---
>> fuse/main.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fuse/main.c b/fuse/main.c
>> index c162912..bc1e496 100644
>> --- a/fuse/main.c
>> +++ b/fuse/main.c
>> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
>>    if (ret)
>>        return ret;
>> 
>> +    if (offset >= vi.i_size)
>> +        return 0;
>> +
>> +    if (offset + size > vi.i_size)
>> +        size = vi.i_size - offset;
>> +
> 
> It would be better to call erofs_pread() with the original offset
> and size (also I think there is some missing memset(0) for
> !EROFS_MAP_MAPPED), and fix up the return value to the needed value.

Yes, that is what I have initially tried. But this way is harder than I thought. EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to represent EOF. So maybe we need a new flag to represent EOF? So that we can distinguish EOF and hole in the middle, and return the bytes read. Or we just abandon the sparse file support, and use EROFS_MAP_MAPPED to indicate EOF exclusively. Since erofs current does not actually support this, right?

> Thanks,
> Gao Xiang
> 
>>    ret = erofs_pread(&vi, buffer, size, offset);
>>    if (ret)
>>        return ret;
>> @@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
>> 
>> static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
>> {
>> -    int ret = erofsfuse_read(path, buffer, size, 0, NULL);
>> +    int ret;
>> +    size_t path_len;
>> +
>> +    erofs_dbg("path:%s size=%zd", path, size);
>> +    ret = erofsfuse_read(path, buffer, size, 0, NULL);
>> 
>>    if (ret < 0)
>>        return ret;
>> +    path_len = min(size - 1, (size_t)ret);
>> +    buffer[path_len] = '\0';
>>    return 0;
>> }
>> 
>> -- 
>> 2.30.0
>> 

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[Gao Xiang]

Hi Weiwen,

On Fri, Jan 22, 2021 at 09:00:44AM +0800, 胡玮文 wrote:
> Hi Xiang,
> 
> > 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@redhat.com> 写道：
> > 
> > Hi Weiwen,
> > 
> >> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> >> readlink should fill a **null terminated** string in buffer.
> >> 
> >> Also, read should return number of bytes remaining on EOF.
> >> 
> >> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> >> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > 
> > Thanks for catching this!
> > 
> >> ---
> >> fuse/main.c | 14 +++++++++++++-
> >> 1 file changed, 13 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/fuse/main.c b/fuse/main.c
> >> index c162912..bc1e496 100644
> >> --- a/fuse/main.c
> >> +++ b/fuse/main.c
> >> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
> >>    if (ret)
> >>        return ret;
> >> 
> >> +    if (offset >= vi.i_size)
> >> +        return 0;
> >> +
> >> +    if (offset + size > vi.i_size)
> >> +        size = vi.i_size - offset;
> >> +
> > 
> > It would be better to call erofs_pread() with the original offset
> > and size (also I think there is some missing memset(0) for
> > !EROFS_MAP_MAPPED), and fix up the return value to the needed value.
> 
> Yes, that is what I have initially tried. But this way is harder than I thought. 
> EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to
> represent EOF. So maybe we need a new flag to represent EOF? 

Nope, I think we just need to handle return value of read, I mean

erofs_ilookup()

ret = erofs_pread()
if (ret)
	return ret;

if (offset + size > vi.i_size)
	return vi.i_size - offset;

return size;

Is that enough? Am I missing something?

> So that we can distinguish EOF and hole in the middle, and return the bytes read.
> Or we just abandon the sparse file support, and use EROFS_MAP_MAPPED to indicate
> EOF exclusively. Since erofs current does not actually support this, right?

Actually, Pratik was working on it months ago, if you have some interest
of uncompressed sparse files (since for compressed files, 0-ed data would
be highly compressed by fixed-sized output compression.), could you pick
his feature up if possible? That would be of great help to EROFS as long
as you have some interest and extra time... :)

https://lore.kernel.org/r/20200102094732.31567-1-pratikshinde320@gmail.com/

Thanks,
Gao Xiang

> 
> > Thanks,
> > Gao Xiang
> > 
> >>    ret = erofs_pread(&vi, buffer, size, offset);
> >>    if (ret)
> >>        return ret;
> >> @@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
> >> 
> >> static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
> >> {
> >> -    int ret = erofsfuse_read(path, buffer, size, 0, NULL);
> >> +    int ret;
> >> +    size_t path_len;
> >> +
> >> +    erofs_dbg("path:%s size=%zd", path, size);
> >> +    ret = erofsfuse_read(path, buffer, size, 0, NULL);
> >> 
> >>    if (ret < 0)
> >>        return ret;
> >> +    path_len = min(size - 1, (size_t)ret);
> >> +    buffer[path_len] = '\0';
> >>    return 0;
> >> }
> >> 
> >> -- 
> >> 2.30.0
> >> 
> 

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[胡玮文]

On Fri, Jan 22, 2021 at 09:49:01AM +0800, Gao Xiang wrote:
> Hi Weiwen,
> 
> On Fri, Jan 22, 2021 at 09:00:44AM +0800, 胡玮文 wrote:
> > Hi Xiang,
> > 
> > > 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@redhat.com> 写道：
> > > 
> > > Hi Weiwen,
> > > 
> > >> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> > >> readlink should fill a **null terminated** string in buffer.
> > >> 
> > >> Also, read should return number of bytes remaining on EOF.
> > >> 
> > >> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> > >> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > 
> > > Thanks for catching this!
> > > 
> > >> ---
> > >> fuse/main.c | 14 +++++++++++++-
> > >> 1 file changed, 13 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/fuse/main.c b/fuse/main.c
> > >> index c162912..bc1e496 100644
> > >> --- a/fuse/main.c
> > >> +++ b/fuse/main.c
> > >> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
> > >>    if (ret)
> > >>        return ret;
> > >> 
> > >> +    if (offset >= vi.i_size)
> > >> +        return 0;
> > >> +
> > >> +    if (offset + size > vi.i_size)
> > >> +        size = vi.i_size - offset;
> > >> +
> > > 
> > > It would be better to call erofs_pread() with the original offset
> > > and size (also I think there is some missing memset(0) for
> > > !EROFS_MAP_MAPPED), and fix up the return value to the needed value.
> > 
> > Yes, that is what I have initially tried. But this way is harder than I thought. 
> > EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to
> > represent EOF. So maybe we need a new flag to represent EOF? 
> 
> Nope, I think we just need to handle return value of read, I mean
> 
> erofs_ilookup()
> 
> ret = erofs_pread()
> if (ret)
> 	return ret;
> 
> if (offset + size > vi.i_size)
> 	return vi.i_size - offset;
> 
> return size;
> 
> Is that enough? Am I missing something?

This should work, except we should also add this branch

if (offset >= vi.i_size)
	return 0;

But how this is better than my original patch? I would say my patch should be
more efficient.

By saying "what I have initially tried" in my last mail, I mean changing
erofs_pread() to return the number of bytes read (just like pread system call,
instead of always 0). I think this is easier to understand for others, but
seems harder to implement. Do you think this is worthful?

> > So that we can distinguish EOF and hole in the middle, and return the bytes read.
> > Or we just abandon the sparse file support, and use EROFS_MAP_MAPPED to indicate
> > EOF exclusively. Since erofs current does not actually support this, right?
> 
> Actually, Pratik was working on it months ago, if you have some interest
> of uncompressed sparse files (since for compressed files, 0-ed data would
> be highly compressed by fixed-sized output compression.), could you pick
> his feature up if possible? That would be of great help to EROFS as long
> as you have some interest and extra time... :)
> 
> https://lore.kernel.org/r/20200102094732.31567-1-pratikshinde320@gmail.com/
> 
> Thanks,
> Gao Xiang
> 
> > 
> > > Thanks,
> > > Gao Xiang
> > > 
> > >>    ret = erofs_pread(&vi, buffer, size, offset);
> > >>    if (ret)
> > >>        return ret;
> > >> @@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
> > >> 
> > >> static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
> > >> {
> > >> -    int ret = erofsfuse_read(path, buffer, size, 0, NULL);
> > >> +    int ret;
> > >> +    size_t path_len;
> > >> +
> > >> +    erofs_dbg("path:%s size=%zd", path, size);
> > >> +    ret = erofsfuse_read(path, buffer, size, 0, NULL);
> > >> 
> > >>    if (ret < 0)
> > >>        return ret;
> > >> +    path_len = min(size - 1, (size_t)ret);
> > >> +    buffer[path_len] = '\0';
> > >>    return 0;
> > >> }
> > >> 
> > >> -- 
> > >> 2.30.0
> > >> 
> > 

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[Gao Xiang]

On Sat, Jan 23, 2021 at 09:18:30PM +0800, 胡玮文 wrote:
> On Fri, Jan 22, 2021 at 09:49:01AM +0800, Gao Xiang wrote:
> > Hi Weiwen,
> > 
> > On Fri, Jan 22, 2021 at 09:00:44AM +0800, 胡玮文 wrote:
> > > Hi Xiang,
> > > 
> > > > 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@redhat.com> 写道：
> > > > 
> > > > Hi Weiwen,
> > > > 
> > > >> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> > > >> readlink should fill a **null terminated** string in buffer.
> > > >> 
> > > >> Also, read should return number of bytes remaining on EOF.
> > > >> 
> > > >> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> > > >> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > > 
> > > > Thanks for catching this!
> > > > 
> > > >> ---
> > > >> fuse/main.c | 14 +++++++++++++-
> > > >> 1 file changed, 13 insertions(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/fuse/main.c b/fuse/main.c
> > > >> index c162912..bc1e496 100644
> > > >> --- a/fuse/main.c
> > > >> +++ b/fuse/main.c
> > > >> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
> > > >>    if (ret)
> > > >>        return ret;
> > > >> 
> > > >> +    if (offset >= vi.i_size)
> > > >> +        return 0;
> > > >> +
> > > >> +    if (offset + size > vi.i_size)
> > > >> +        size = vi.i_size - offset;
> > > >> +
> > > > 
> > > > It would be better to call erofs_pread() with the original offset
> > > > and size (also I think there is some missing memset(0) for
> > > > !EROFS_MAP_MAPPED), and fix up the return value to the needed value.
> > > 
> > > Yes, that is what I have initially tried. But this way is harder than I thought. 
> > > EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to
> > > represent EOF. So maybe we need a new flag to represent EOF? 
> > 
> > Nope, I think we just need to handle return value of read, I mean
> > 
> > erofs_ilookup()
> > 
> > ret = erofs_pread()
> > if (ret)
> > 	return ret;
> > 
> > if (offset + size > vi.i_size)
> > 	return vi.i_size - offset;
> > 
> > return size;
> > 
> > Is that enough? Am I missing something?
> 
> This should work, except we should also add this branch
> 
> if (offset >= vi.i_size)
> 	return 0;

yeah, agreed. It'd also be added after erofs_pread().

>
> But how this is better than my original patch? I would say my patch should be
> more efficient.
> 
> By saying "what I have initially tried" in my last mail, I mean changing
> erofs_pread() to return the number of bytes read (just like pread system call,
> instead of always 0). I think this is easier to understand for others, but
> seems harder to implement. Do you think this is worthful?
> 

There are 2 reasons for me to do it at least:
1) need to memset(0) for these unmapped buffers;
2) introduce a hook to fs to read data regardless of i_size,
   just as linux kernel page cache approach.

Don't be confused with ->i_size (this is only a EOF-marker) and
the real inode data, that are two different concepts for me, I'd
like to handle all data processing in erofs_pread() (even for
post-EOF case), but only deal with i_size in erofsfuse_read().

Thanks,
Gao Xiang

Re: [PATCH] erofs-utils: fuse: fix random readlink error

[Gao Xiang]

On Sat, Jan 23, 2021 at 11:22:13PM +0800, Gao Xiang wrote:
> On Sat, Jan 23, 2021 at 09:18:30PM +0800, 胡玮文 wrote:
> > On Fri, Jan 22, 2021 at 09:49:01AM +0800, Gao Xiang wrote:
> > > Hi Weiwen,
> > > 
> > > On Fri, Jan 22, 2021 at 09:00:44AM +0800, 胡玮文 wrote:
> > > > Hi Xiang,
> > > > 
> > > > > 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@redhat.com> 写道：
> > > > > 
> > > > > Hi Weiwen,
> > > > > 
> > > > >> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> > > > >> readlink should fill a **null terminated** string in buffer.
> > > > >> 
> > > > >> Also, read should return number of bytes remaining on EOF.
> > > > >> 
> > > > >> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> > > > >> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > > > 
> > > > > Thanks for catching this!
> > > > > 
> > > > >> ---
> > > > >> fuse/main.c | 14 +++++++++++++-
> > > > >> 1 file changed, 13 insertions(+), 1 deletion(-)
> > > > >> 
> > > > >> diff --git a/fuse/main.c b/fuse/main.c
> > > > >> index c162912..bc1e496 100644
> > > > >> --- a/fuse/main.c
> > > > >> +++ b/fuse/main.c
> > > > >> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
> > > > >>    if (ret)
> > > > >>        return ret;
> > > > >> 
> > > > >> +    if (offset >= vi.i_size)
> > > > >> +        return 0;
> > > > >> +
> > > > >> +    if (offset + size > vi.i_size)
> > > > >> +        size = vi.i_size - offset;
> > > > >> +
> > > > > 
> > > > > It would be better to call erofs_pread() with the original offset
> > > > > and size (also I think there is some missing memset(0) for
> > > > > !EROFS_MAP_MAPPED), and fix up the return value to the needed value.
> > > > 
> > > > Yes, that is what I have initially tried. But this way is harder than I thought. 
> > > > EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to
> > > > represent EOF. So maybe we need a new flag to represent EOF? 
> > > 
> > > Nope, I think we just need to handle return value of read, I mean
> > > 
> > > erofs_ilookup()
> > > 
> > > ret = erofs_pread()
> > > if (ret)
> > > 	return ret;
> > > 
> > > if (offset + size > vi.i_size)
> > > 	return vi.i_size - offset;
> > > 
> > > return size;
> > > 
> > > Is that enough? Am I missing something?
> > 
> > This should work, except we should also add this branch
> > 
> > if (offset >= vi.i_size)
> > 	return 0;
> 
> yeah, agreed. It'd also be added after erofs_pread().
> 
> >
> > But how this is better than my original patch? I would say my patch should be
> > more efficient.
> > 
> > By saying "what I have initially tried" in my last mail, I mean changing
> > erofs_pread() to return the number of bytes read (just like pread system call,
> > instead of always 0). I think this is easier to understand for others, but
> > seems harder to implement. Do you think this is worthful?
> > 
> 
> There are 2 reasons for me to do it at least:
> 1) need to memset(0) for these unmapped buffers;
> 2) introduce a hook to fs to read data regardless of i_size,
>    just as linux kernel page cache approach.
> 
> Don't be confused with ->i_size (this is only a EOF-marker) and
> the real inode data, that are two different concepts for me, I'd
> like to handle all data processing in erofs_pread() (even for
> post-EOF case), but only deal with i_size in erofsfuse_read().
> 

Also, no need to follow erofs_pread() as the pread() system call.
It just used a similiar name. The core concept of this is to handle
file data itself. There is nothing to do with eof-marker (but much
related to inode extent/block mapping instead, although currently
EROFS extent mapping format internally relies on i_size.)

Thanks,
Gao Xiang

> Thanks,
> Gao Xiang
> 

[PATCH v2] erofs-utils: fuse: fix random readlink error

[Hu Weiwen]

readlink should fill a **null-terminated** string in the buffer.

To achieve this:
1) memset(0) for unmapped extents;
2) make erofsfuse_read() properly returning the actual bytes read;
3) insert a null character if the path is truncated.

Link: https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
---
 fuse/main.c | 8 ++++++++
 lib/data.c  | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/fuse/main.c b/fuse/main.c
index c162912..70558b0 100644
--- a/fuse/main.c
+++ b/fuse/main.c
@@ -74,6 +74,10 @@ static int erofsfuse_read(const char *path, char *buffer,
 	ret = erofs_pread(&vi, buffer, size, offset);
 	if (ret)
 		return ret;
+	if (offset + size > vi.i_size)
+		return vi.i_size - offset;
+	if (offset >= vi.i_size)
+		return 0;
 	return size;
 }
 
@@ -83,6 +87,10 @@ static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
 
 	if (ret < 0)
 		return ret;
+	DBG_BUGON(ret > size);
+	if (ret == size)
+		buffer[size - 1] = '\0';
+	erofs_dbg("readlink result %s", buffer);
 	return 0;
 }
 
diff --git a/lib/data.c b/lib/data.c
index 3781846..3641536 100644
--- a/lib/data.c
+++ b/lib/data.c
@@ -29,6 +29,7 @@ static int erofs_map_blocks_flatmode(struct erofs_inode *inode,
 	if (offset >= inode->i_size) {
 		/* leave out-of-bound access unmapped */
 		map->m_flags = 0;
+		map->m_plen = 0;
 		goto out;
 	}
 
@@ -91,9 +92,13 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 
 		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
 			if (!map.m_llen) {
+				/* reached EOF */
+				memset(buffer + ptr - offset, 0,
+				       offset + size - ptr);
 				ptr = offset + size;
 				continue;
 			}
+			memset(buffer + map.m_la - offset, 0, map.m_llen);
 			ptr = map.m_la + map.m_llen;
 			continue;
 		}
@@ -138,6 +143,7 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 			return ret;
 
 		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
+			memset(buffer + map.m_la - offset, 0, map.m_llen);
 			end = map.m_la;
 			continue;
 		}
-- 
2.25.1

Re: [PATCH v2] erofs-utils: fuse: fix random readlink error

[Gao Xiang via Linux-erofs]

Hi Weiwen,

On Sat, Jan 30, 2021 at 02:07:47AM +0800, Hu Weiwen wrote:
> readlink should fill a **null-terminated** string in the buffer.
> 
> To achieve this:
> 1) memset(0) for unmapped extents;
> 2) make erofsfuse_read() properly returning the actual bytes read;
> 3) insert a null character if the path is truncated.
> 
> Link: https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn

Looked into this patch just now...

The Link tag is only used for refering to the patch itself.

> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> ---

...

>  
> @@ -91,9 +92,13 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
>  
>  		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
>  			if (!map.m_llen) {
> +				/* reached EOF */
> +				memset(buffer + ptr - offset, 0,
> +				       offset + size - ptr);
>  				ptr = offset + size;
>  				continue;
>  			}
> +			memset(buffer + map.m_la - offset, 0, map.m_llen);

There might be some minor issue --- `offset' could be larger than
`map.m_la' if sparse file is supported then.

I made an update version of this to fix these (some cleanup is
included as well), it would be nice of you to take another look at
this as well...

Thanks,
Gao Xiang

From bfbd8ee056aca57a77034b8723f3f828f806747b Mon Sep 17 00:00:00 2001
From: Hu Weiwen <sehuww@mail.scut.edu.cn>
Date: Sat, 30 Jan 2021 02:07:47 +0800
Subject: [PATCH v3] erofs-utils: fuse: fix random readlink error

readlink should fill a **null-terminated** string in the buffer [1].

To achieve this:
1) memset(0) for unmapped extents;
2) make erofsfuse_read() properly returning the actual bytes read;
3) insert a null character if the path is truncated.

[1] https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
 fuse/main.c |  8 ++++++++
 lib/data.c  | 20 ++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/fuse/main.c b/fuse/main.c
index c16291272e75..37119ea8728d 100644
--- a/fuse/main.c
+++ b/fuse/main.c
@@ -74,6 +74,10 @@ static int erofsfuse_read(const char *path, char *buffer,
 	ret = erofs_pread(&vi, buffer, size, offset);
 	if (ret)
 		return ret;
+	if (offset + size > vi.i_size)
+		return vi.i_size - offset;
+	if (offset >= vi.i_size)
+		return 0;
 	return size;
 }
 
@@ -83,6 +87,10 @@ static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
 
 	if (ret < 0)
 		return ret;
+	DBG_BUGON(ret > size);
+	if (ret == size)
+		buffer[size - 1] = '\0';
+	erofs_dbg("readlink(%s): %s", path, buffer);
 	return 0;
 }
 
diff --git a/lib/data.c b/lib/data.c
index 3781846743aa..5bc525f19826 100644
--- a/lib/data.c
+++ b/lib/data.c
@@ -29,6 +29,7 @@ static int erofs_map_blocks_flatmode(struct erofs_inode *inode,
 	if (offset >= inode->i_size) {
 		/* leave out-of-bound access unmapped */
 		map->m_flags = 0;
+		map->m_plen = 0;
 		goto out;
 	}
 
@@ -80,6 +81,7 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 	erofs_off_t ptr = offset;
 
 	while (ptr < offset + size) {
+		char *const estart = buffer + ptr - offset;
 		erofs_off_t eend;
 
 		map.m_la = ptr;
@@ -89,29 +91,30 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 
 		DBG_BUGON(map.m_plen != map.m_llen);
 
+		/* trim extent */
+		eend = min(offset + size, map.m_la + map.m_llen);
+		DBG_BUGON(ptr < map.m_la);
+
 		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
 			if (!map.m_llen) {
+				/* reached EOF */
+				memset(estart, 0, offset + size - ptr);
 				ptr = offset + size;
 				continue;
 			}
-			ptr = map.m_la + map.m_llen;
+			memset(estart, 0, eend - ptr);
+			ptr = eend;
 			continue;
 		}
 
-		/* trim extent */
-		eend = min(offset + size, map.m_la + map.m_llen);
-		DBG_BUGON(ptr < map.m_la);
-
 		if (ptr > map.m_la) {
 			map.m_pa += ptr - map.m_la;
 			map.m_la = ptr;
 		}
 
-		ret = dev_read(buffer + ptr - offset,
-			       map.m_pa, eend - map.m_la);
+		ret = dev_read(estart, map.m_pa, eend - map.m_la);
 		if (ret < 0)
 			return -EIO;
-
 		ptr = eend;
 	}
 	return 0;
@@ -138,6 +141,7 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 			return ret;
 
 		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
+			memset(buffer + map.m_la - offset, 0, map.m_llen);
 			end = map.m_la;
 			continue;
 		}
-- 
2.24.0

Re: [PATCH v3] erofs-utils: fuse: fix random readlink error

[胡玮文]

On Wed, Feb 10, 2021 at 03:38:50AM +0800, Gao Xiang wrote:

...

> From bfbd8ee056aca57a77034b8723f3f828f806747b Mon Sep 17 00:00:00 2001
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Date: Sat, 30 Jan 2021 02:07:47 +0800
> Subject: [PATCH v3] erofs-utils: fuse: fix random readlink error
> 
> readlink should fill a **null-terminated** string in the buffer [1].
> 
> To achieve this:
> 1) memset(0) for unmapped extents;
> 2) make erofsfuse_read() properly returning the actual bytes read;
> 3) insert a null character if the path is truncated.
> 
> [1] https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>

It looks good. I've run this patched version against previously failing tests.

Also now I understand the purpose of erofs_map_blocks_flatmode() better.

Thanks,
Hu Weiwen

Re: [PATCH v2] erofs-utils: fuse: fix random readlink error

[Li GuiFu via Linux-erofs]

On 2021/2/10 3:38, Gao Xiang via Linux-erofs wrote:
> Hi Weiwen,
> 
> On Sat, Jan 30, 2021 at 02:07:47AM +0800, Hu Weiwen wrote:
>> readlink should fill a **null-terminated** string in the buffer.
>>
>> To achieve this:
>> 1) memset(0) for unmapped extents;
>> 2) make erofsfuse_read() properly returning the actual bytes read;
>> 3) insert a null character if the path is truncated.
>>
>> Link: https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
> 
> Looked into this patch just now...
> 
> The Link tag is only used for refering to the patch itself.
> 
>> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
>> ---
> 
> ...
> 
>>  
>> @@ -91,9 +92,13 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
>>  
>>  		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
>>  			if (!map.m_llen) {
>> +				/* reached EOF */
>> +				memset(buffer + ptr - offset, 0,
>> +				       offset + size - ptr);
>>  				ptr = offset + size;
>>  				continue;
>>  			}
>> +			memset(buffer + map.m_la - offset, 0, map.m_llen);
> 
> There might be some minor issue --- `offset' could be larger than
> `map.m_la' if sparse file is supported then.
> 
> I made an update version of this to fix these (some cleanup is
> included as well), it would be nice of you to take another look at
> this as well...
> 
> Thanks,
> Gao Xiang
> 
> From bfbd8ee056aca57a77034b8723f3f828f806747b Mon Sep 17 00:00:00 2001
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Date: Sat, 30 Jan 2021 02:07:47 +0800
> Subject: [PATCH v3] erofs-utils: fuse: fix random readlink error
> 
> readlink should fill a **null-terminated** string in the buffer [1].
> 
> To achieve this:
> 1) memset(0) for unmapped extents;
> 2) make erofsfuse_read() properly returning the actual bytes read;
> 3) insert a null character if the path is truncated.
> 
> [1] https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
> Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
>  fuse/main.c |  8 ++++++++
>  lib/data.c  | 20 ++++++++++++--------
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
It looks good
Reviewed-by: Li Guifu <bluce.lee@aliyun.com>

Thanks,

[PATCH v4] erofs-utils: fuse: fix random readlink error

[Gao Xiang via Linux-erofs]

From: Hu Weiwen <sehuww@mail.scut.edu.cn>

readlink should fill a **null-terminated** string in the buffer [1].

To achieve this:
1) memset(0) for unmapped extents;
2) make erofsfuse_read() properly returning the actual bytes read;
3) insert a null character if the path is truncated.

[1] https://lore.kernel.org/r/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn
Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn>
Reviewed-by: Li Guifu <bluce.lee@aliyun.com>
Signed-off-by: Gao Xiang <hsiangkao@aol.com>
---
changes since v3:
 - fix z_erofs_read_data() buffer range as well.

 fuse/main.c |  8 ++++++++
 lib/data.c  | 46 +++++++++++++++++++++++++---------------------
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/fuse/main.c b/fuse/main.c
index c16291272e75..37119ea8728d 100644
--- a/fuse/main.c
+++ b/fuse/main.c
@@ -74,6 +74,10 @@ static int erofsfuse_read(const char *path, char *buffer,
 	ret = erofs_pread(&vi, buffer, size, offset);
 	if (ret)
 		return ret;
+	if (offset + size > vi.i_size)
+		return vi.i_size - offset;
+	if (offset >= vi.i_size)
+		return 0;
 	return size;
 }
 
@@ -83,6 +87,10 @@ static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
 
 	if (ret < 0)
 		return ret;
+	DBG_BUGON(ret > size);
+	if (ret == size)
+		buffer[size - 1] = '\0';
+	erofs_dbg("readlink(%s): %s", path, buffer);
 	return 0;
 }
 
diff --git a/lib/data.c b/lib/data.c
index 3781846743aa..56de16b3c840 100644
--- a/lib/data.c
+++ b/lib/data.c
@@ -29,6 +29,7 @@ static int erofs_map_blocks_flatmode(struct erofs_inode *inode,
 	if (offset >= inode->i_size) {
 		/* leave out-of-bound access unmapped */
 		map->m_flags = 0;
+		map->m_plen = 0;
 		goto out;
 	}
 
@@ -80,6 +81,7 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 	erofs_off_t ptr = offset;
 
 	while (ptr < offset + size) {
+		char *const estart = buffer + ptr - offset;
 		erofs_off_t eend;
 
 		map.m_la = ptr;
@@ -89,29 +91,30 @@ static int erofs_read_raw_data(struct erofs_inode *inode, char *buffer,
 
 		DBG_BUGON(map.m_plen != map.m_llen);
 
+		/* trim extent */
+		eend = min(offset + size, map.m_la + map.m_llen);
+		DBG_BUGON(ptr < map.m_la);
+
 		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
 			if (!map.m_llen) {
+				/* reached EOF */
+				memset(estart, 0, offset + size - ptr);
 				ptr = offset + size;
 				continue;
 			}
-			ptr = map.m_la + map.m_llen;
+			memset(estart, 0, eend - ptr);
+			ptr = eend;
 			continue;
 		}
 
-		/* trim extent */
-		eend = min(offset + size, map.m_la + map.m_llen);
-		DBG_BUGON(ptr < map.m_la);
-
 		if (ptr > map.m_la) {
 			map.m_pa += ptr - map.m_la;
 			map.m_la = ptr;
 		}
 
-		ret = dev_read(buffer + ptr - offset,
-			       map.m_pa, eend - map.m_la);
+		ret = dev_read(estart, map.m_pa, eend - map.m_la);
 		if (ret < 0)
 			return -EIO;
-
 		ptr = eend;
 	}
 	return 0;
@@ -137,19 +140,6 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 		if (ret)
 			return ret;
 
-		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
-			end = map.m_la;
-			continue;
-		}
-
-		ret = dev_read(raw, map.m_pa, EROFS_BLKSIZ);
-		if (ret < 0)
-			return -EIO;
-
-		algorithmformat = map.m_flags & EROFS_MAP_ZIPPED ?
-						Z_EROFS_COMPRESSION_LZ4 :
-						Z_EROFS_COMPRESSION_SHIFTED;
-
 		/*
 		 * trim to the needed size if the returned extent is quite
 		 * larger than requested, and set up partial flag as well.
@@ -171,6 +161,20 @@ static int z_erofs_read_data(struct erofs_inode *inode, char *buffer,
 			end = map.m_la;
 		}
 
+		if (!(map.m_flags & EROFS_MAP_MAPPED)) {
+			memset(buffer + end - offset, 0, length);
+			end = map.m_la;
+			continue;
+		}
+
+		ret = dev_read(raw, map.m_pa, EROFS_BLKSIZ);
+		if (ret < 0)
+			return -EIO;
+
+		algorithmformat = map.m_flags & EROFS_MAP_ZIPPED ?
+						Z_EROFS_COMPRESSION_LZ4 :
+						Z_EROFS_COMPRESSION_SHIFTED;
+
 		ret = z_erofs_decompress(&(struct z_erofs_decompress_req) {
 					.in = raw,
 					.out = buffer + end - offset,
-- 
2.24.0