RE: [PATCH v3 07/17] hw/9pfs: Support getting current directory offset for Windows

[Shi, Guohuai]

> -----Original Message-----
> From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Sent: Wednesday, December 28, 2022 19:51
> To: Greg Kurz <groug@kaod.org>; qemu-devel@nongnu.org
> Cc: Meng, Bin <Bin.Meng@windriver.com>; Shi, Guohuai
> <Guohuai.Shi@windriver.com>
> Subject: Re: [PATCH v3 07/17] hw/9pfs: Support getting current directory
> offset for Windows
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
> 
> On Wednesday, December 21, 2022 7:02:43 PM CET Shi, Guohuai wrote:
> >
> > > -----Original Message-----
> > > From: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Sent: Wednesday, December 21, 2022 22:48
> > > To: Greg Kurz <groug@kaod.org>; qemu-devel@nongnu.org
> > > Cc: Shi, Guohuai <Guohuai.Shi@windriver.com>; Meng, Bin
> > > <Bin.Meng@windriver.com>
> > > Subject: Re: [PATCH v3 07/17] hw/9pfs: Support getting current
> > > directory offset for Windows
> > >
> > > CAUTION: This email comes from a non Wind River email account!
> > > Do not click links or open attachments unless you recognize the
> > > sender and know the content is safe.
> > >
> > > On Monday, December 19, 2022 11:20:11 AM CET Bin Meng wrote:
> > > > From: Guohuai Shi <guohuai.shi@windriver.com>
> > > >
> > > > On Windows 'struct dirent' does not have current directory offset.
> > > > Update qemu_dirent_off() to support Windows.
> > > >
> > > > While we are here, add a build time check to error out if a new
> > > > host does not implement this helper.
> > > >
> > > > Signed-off-by: Guohuai Shi <guohuai.shi@windriver.com>
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  hw/9pfs/9p-util.h       | 11 ++++++++---
> > > >  hw/9pfs/9p-util-win32.c |  7 +++++++
> > > >  hw/9pfs/9p.c            |  4 ++--
> > > >  hw/9pfs/codir.c         |  2 +-
> > > >  4 files changed, 18 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > 90420a7578..e395936b30 100644
> > > > --- a/hw/9pfs/9p-util.h
> > > > +++ b/hw/9pfs/9p-util.h
> > > > @@ -127,6 +127,7 @@ int unlinkat_win32(int dirfd, const char
> > > > *pathname, int flags);  int statfs_win32(const char *root_path,
> > > > struct statfs *stbuf);  int openat_dir(int dirfd, const char
> > > > *name);  int openat_file(int dirfd, const char *name, int flags,
> > > > mode_t mode);
> > > > +off_t qemu_dirent_off_win32(void *s, void *fs);
> > > >  #endif
> > > >
> > > >  static inline void close_preserve_errno(int fd) @@ -200,12
> > > > +201,16 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char
> *filename,
> > > >   * so ensure it is manually injected earlier and call here when
> > > >   * needed.
> > > >   */
> > > > -static inline off_t qemu_dirent_off(struct dirent *dent)
> > > > +static inline off_t qemu_dirent_off(struct dirent *dent, void *s,
> > > > +void *fs)
> > > >  {
> > >
> > > Not sure why you chose void* here.
> >
> > It is "V9fsState *" here, but 9p-util.h may be included by some other
> source file which is not have definition of "V9fsState".
> > So change it to "void *" to prevent unnecessary type declarations.
> 
> You can anonymously declare the struct to avoid cyclic dependencies (e.g.
> like in 9p.h):
> 
> typedef struct V9fsState V9fsState;
> 
> Avoid the void.
> 

Got it, understood.

> > >
> > > > -#ifdef CONFIG_DARWIN
> > > > +#if defined(CONFIG_DARWIN)
> > > >      return dent->d_seekoff;
> > > > -#else
> > > > +#elif defined(CONFIG_LINUX)
> > > >      return dent->d_off;
> > > > +#elif defined(CONFIG_WIN32)
> > > > +    return qemu_dirent_off_win32(s, fs); #else #error Missing
> > > > +qemu_dirent_off() implementation for this host system
> > > >  #endif
> > > >  }
> > > >
> > > > diff --git a/hw/9pfs/9p-util-win32.c b/hw/9pfs/9p-util-win32.c
> > > > index 7a270a7bd5..3592e057ce 100644
> > > > --- a/hw/9pfs/9p-util-win32.c
> > > > +++ b/hw/9pfs/9p-util-win32.c
> > > > @@ -929,3 +929,10 @@ int qemu_mknodat(int dirfd, const char
> > > > *filename,
> > > mode_t mode, dev_t dev)
> > > >      errno = ENOTSUP;
> > > >      return -1;
> > > >  }
> > > > +
> > > > +off_t qemu_dirent_off_win32(void *s, void *fs) {
> > > > +    V9fsState *v9fs = s;
> > > > +
> > > > +    return v9fs->ops->telldir(&v9fs->ctx, (V9fsFidOpenState
> > > > + *)fs); }
> > >
> > > That's a bit tricky. So far (i.e. for Linux host, macOS host) we are
> > > storing the directory offset directly to the dirent struct. We are
> > > not using
> > > telldir() as the stream might have mutated in the meantime, hence
> > > calling
> > > telldir() might return a value that does no longer correlate to
> > > dirent in question.
> > >
> > > Hence my idea was to use the same hack for Windows as we did for
> > > macOS, where we simply misuse a dirent field known to be not used,
> > > on macOS that's d_seekoff which we are misusing for that purpose.
> > >
> > > Looking at the mingw dirent.h header though, there is not much we
> > > can choose from. d_reclen is not used, but too short, d_ino is not
> > > used by mingw, but currently we actually read this field in server
> > > common code to assemble a file ID for guest. I mean it is always
> > > zero on Windows, so we could still misuse it, but we would need to
> > > inject more hacks there. :/
> >
> > If you check seekdir and telldir() implement in MinGW, you can see
> > that MinGW (and Windows) does not have a safety way to seek/tell directory
> offset.
> > Because Windows POSIX and native API does not provide a way to seek
> directory.
> > Windows native API only allow to read directory forward, but not backward.
> > So even we store the d_seekoff to some places, the directory may still be
> modified.
> >
> > And Windows file system actually has inode number, MinGW does not introduce
> it to MinGW API.
> > So I think it is not good way to use d_ino.
> 
> Scratch that d_ino hack. My original concern was that we might get
> concurrency on the directory stream, however after a recap I stumbled over
> one of my comments on this:
> 
> static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
>                            struct V9fsDirEnt **entries, off_t offset,
>                            int32_t maxsize, bool dostat) {
>     ...
>     /*
>      * TODO: Here should be a warn_report_once() if lock failed.
>      *
>      * With a good 9p client we should not get into concurrency here,
>      * because a good client would not use the same fid for concurrent
>      * requests. We do the lock here for safety reasons though. However
>      * the client would then suffer performance issues, so better log that
>      * issue here.
>      */
>     v9fs_readdir_lock(&fidp->fs.dir);
>     ...
> }
> 
> So it boils down to whether or not we would want to care about broken 9p
> clients on Windows host or not, and considering the huge amount of code to
> deal with here for Windows host, we might say that we have more important
> (real) problems to deal with than caring about a broken 9p client that does
> not clone a FID before sending Treaddir (9p2000.L) or Tread (9p2000.u).
> 
> However looking at current MinGW implementation I start to think whether we
> should use that API for directory retrieval at all, because for seekdir()
> they are rewinding the directory stream to the very beginning on *each* call
> of
> seekdir() and then readdir() in a while loop to the requested location for
> which they use a simple, self-incremented consecutive number as stream
> position:
> 
> https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-
> crt/misc/dirent.c#L319
> 
> Which can lead to two problems:
> 
> 1. n-square performance issue (minor issue).
> 
> 2. Inconsistent state if directory is modified in between (major issue), e.g.
> the same directory appearing more than once in output, or directories not
> appearing at all in output even though they were neither newly created nor
> deleted. POSIX does not define what happens with deleted or newly created
> directories in between, but it guarantees a consistent state.
> 
> What about calling _findfirst() and _findnext() directly, fetching all
> directory entries at once, closing the stream, and 9p would just access that
> snapshot instead? As long as the stream is not closed, Windows blocks all
> directory changes, so we would have a consistent state.
> 

I met the issue #2 you mentioned, you can see the comments I added in function 
local_seekdir().
Fetching all directory entries can resolve this issue, but it may need to 
allocate large memory if there are many entries.
And we also need to re-write local_telldir(), local_rewinddir(), 
local_opendir(), local_closedir().
It would be a big change for 9pfs.

> > >
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index
> > > > 072cf67956..be247eeb30
> > > > 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -2336,7 +2336,7 @@ static int coroutine_fn
> > > v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> > > >          count += len;
> > > >          v9fs_stat_free(&v9stat);
> > > >          v9fs_path_free(&path);
> > > > -        saved_dir_pos = qemu_dirent_off(dent);
> > > > +        saved_dir_pos = qemu_dirent_off(dent, pdu->s, &fidp->fs);
> > > >      }
> > > >
> > > >      v9fs_readdir_unlock(&fidp->fs.dir);
> > > > @@ -2537,7 +2537,7 @@ static int coroutine_fn
> > > > v9fs_do_readdir(V9fsPDU *pdu,
> > > V9fsFidState *fidp,
> > > >              qid.version = 0;
> > > >          }
> > > >
> > > > -        off = qemu_dirent_off(dent);
> > > > +        off = qemu_dirent_off(dent, pdu->s, &fidp->fs);
> > > >          v9fs_string_init(&name);
> > > >          v9fs_string_sprintf(&name, "%s", dent->d_name);
> > > >
> > > > diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c index
> > > > 93ba44fb75..d40515a607 100644
> > > > --- a/hw/9pfs/codir.c
> > > > +++ b/hw/9pfs/codir.c
> > > > @@ -168,7 +168,7 @@ static int do_readdir_many(V9fsPDU *pdu,
> > > > V9fsFidState
> > > *fidp,
> > > >          }
> > > >
> > > >          size += len;
> > > > -        saved_dir_pos = qemu_dirent_off(dent);
> > > > +        saved_dir_pos = qemu_dirent_off(dent, s, &fidp->fs);
> > > >      }
> > > >
> > > >      /* restore (last) saved position */
> > > >
> > >
> > >
> >
> >
> >
>