On Tue, 26 Nov 2024 17:03:45 +0100
Christian Schoenebeck <[email protected]> wrote:
> 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 <[email protected]>
> > ---
> > 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
>
>
Great explanation Christian !
Reviewed-by: Greg Kurz <[email protected]>
Cheers,
--
Greg
