Re: [PATCH 5/6] 9pfs: fix 'Tgetattr' after unlink

[Christian Schoenebeck]

On Sunday, November 24, 2024 4:50:03 PM CET Christian Schoenebeck wrote:
> With a valid file ID (FID) of an open file, it should be possible to send
> a 'Tgettattr' 9p request and successfully receive a 'Rgetattr' response,
> even if the file has been removed in the meantime. Currently this would
> fail with ENOENT.
> 
> I.e. this fixes the following misbehaviour with a 9p Linux client:
> 
>   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
>   unlink("/home/tst/filename") = 0
>   fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or directory)
> 
> Expected results:
> 
>   open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
>   unlink("/home/tst/filename") = 0
>   fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
> 
> This is because 9p server is always using a path name based stat() call
> which fails as soon as the file got removed. So to fix this, use fstat()
> whenever we have an open file descriptor already.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/103
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 851e36b9a1..578517739a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1596,7 +1596,13 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>          retval = -ENOENT;
>          goto out_nofid;
>      }
> -    retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> +    if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> +        (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
> +    {
> +        retval = v9fs_co_fstat(pdu, fidp, &stbuf);
> +    } else {
> +        retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> +    }

As for performance fstat() vs. lstat(): with glibc >= 2.39 and/or Linux
kernel >= 6.6, fstat() is Theta(1) whereas lstat() is O(log n). So fstat() is
faster than lstat() and hence prioritizing fstat() over lstat() does make
sense here IMO.

That's because on Linux kernel side fstat() is implemented by a simple
constant time linear array access via file descriptor number, whereas lstat()
needs to lookup the path and hence walk a tree.

There is a caveat though: Both on glibc and Linux kernel side there was a
performance bug each, which were both fixed in September 2023 by glibc 2.39
and Linux kernel 6.6 respectively:

kernel fix: https://github.com/torvalds/linux/commit/9013c51

glibc fix: https://github.com/bminor/glibc/commit/551101e

So on glibc side, due to a misconception, they inappropriately translated
fstat(fd, buf) -> fstatat(fd, "", buf, AT_EMPTY_PATH) for a long time, instead
of just calling fstat() directly as ought to be and done now.

And on kernel side, the negative performance impact of case AT_EMPTY_PATH +
empty string wasn't considered in fstatat() implementation. This case is now
short-circuited right at the beginning of the function.

/Christian