Re: [PATCH] Migrating storeio from trivfs to netfs.

Mikhail Karpov, le mer. 26 août 2026 18:55:23 +0700, a ecrit:
> > > diff --git a/storeio/dev.c b/storeio/dev.c
> > > index c87400c..97b36cd 100644
> > > --- a/storeio/dev.c
> > > +++ b/storeio/dev.c
> > > @@ -22,7 +22,6 @@
> > >  #include <assert-backtrace.h>
> > >  #include <string.h>
> > >  #include <hurd/pager.h>
> > > -#include <hurd/store.h>
> >
> > dev.c is still calling store_create etc. so should include store.h
> 
> Is this necessary? dev.h already includes store.h.

It is not necessary, but somebody might want to try to stop including
hurd/store.h from dev.h (to avoid rebuilding includers of dev.h when
hurd/store.h is modified) and conclude it should still include it just
because it breaks building dev.c, which the real fix would really be to
move the inclusion into dev.c. Better pave the way for that: what a file
needs, it includes headers for that, so that only needed headers are
included.

> > > diff --git a/storeio/pager.c b/storeio/pager.c
> > > index 11bf469..7df1a79 100644
> > > --- a/storeio/pager.c
> > > +++ b/storeio/pager.c
> > > @@ -27,6 +27,7 @@
> > >  #include <error.h>
> > >  #include <sys/mman.h>
> > >  #include <stdio.h>
> > > +#include <string.h>
> > >
> > >  #include "dev.h"
> > >  
> > > @@ -51,6 +52,11 @@ pager_read_page (struct user_pager_info *upi,
> > >      /* Read a partial page if necessary to avoid reading off the end. 
> */
> > >      want = store->size - page;
> > >
> > > +  void *new_buf = mmap (0, want, PROT_READ|PROT_WRITE, MAP_ANON, 0, 0);
> > > +  if (new_buf == MAP_FAILED)
> > > +    return EIO;
> > > +
> > > +  *((void **) buf) = new_buf;
> > >    err = dev_read (dev, page, want, (void **)buf, &read);
> >
> > Mmm, why always allocating? It's better to allocate lazily in case the
> > read can be directly be fed by raw_read i.e. store_read, and avoid
> > a copy. That means keeping ensure_buf() etc., did you have a reason
> > to remove it? You can make pager_read_page set buf to NULL, and let
> > dev_read cope with that case, and otherwise try to use the provided
> > buffer (netfs_attempt_read case).
> 
> I think we can allocate memory in the pager_read_page function if it hasn't
> already been allocated.

My point is that it would miss the opportunity to let functions further
down do the allocation. In the current dev_read, when we call raw_read
and there call store_read, it's store_read that can allocate it, which
itself can delegate the allocation to the read method of the class etc.
possibly up to a function that is happier to be able to provide an
already-allocated buffer, rather than having to copy data into the
buffer allocated by pager_read_page.

> This way, dev_read will behave consistently.

The buffer-return semantic explained above is a usual thing in the Hurd
and avoids various copies.

Samuel