Re: [PATCH] netfs: Update main API document

David Howells
2025-04-09T09:05:42Z

Bagas Sanjaya <bagasdotme@gmail.com> wrote:

> > + * For writeback, it is unknown how much there will be to write until the
> "... will be written ..."
> > + pagecache is walked, so no limit is set by the library.

No, I mean "how much there will be to write" - ie. how much dirty data there
is in the pagecache.

> > +Further, if a read from the cache fails, the library will ask the filesystem to
> > +do the read instead, renegotiating and retiling the subrequests as necessary.
> Read from the filesystem itself or direct read?

I'm not sure what you mean. Here, I'm talking about read subrequests - i.e. a
subrequest that corresponds to a BIO issued to the cache or a single RPC
issued to the server. Things like DIO and pagecache are at a higher level and
not directly exposed to the filesystem.

Maybe I should amend the text to read:

Further, if one or more subrequests issued to read from the cache
fail, the library will issue them to the filesystem instead,
renegotiating and retiling the subrequests as necessary.

> > +Netfslib will pin resources on an inode for future writeback (such as pinning
> > +use of an fscache cookie) when an inode is dirtied. However, this needs
> > +managing. Firstly, a function is provided to unpin the writeback in
> inode management?
> > +``->write_inode()``::

Is "inode management" meant to be a suggested insertion or an alternative for
the subsection title?

> > -The above fields are the ones the netfs can use. They are:
> > +They are:
> "These fields are, in detail:"

It feels unnecessarily repetitive to say "these fields", but "they are" also
sounds stilted. How about I rearrange things a little.

The request structure manages the request as a whole, holding some resources
and state on behalf of the filesystem and tracking the collection of results::

struct netfs_io_request {
enum netfs_io_origin origin;
struct inode *inode;
struct address_space *mapping;
struct netfs_group *group;
struct netfs_io_stream io_streams[];
void *netfs_priv;
void *netfs_priv2;
unsigned long long start;
unsigned long long len;
unsigned long long i_size;
unsigned int debug_id;
unsigned long flags;
...
};

Many of the fields are for internal use, but the fields shown here are of
interest to the filesystem:

* ``origin``
...

And then put the bit about wrapping the struct after the field explanation:

If the filesystem wants more private data than is afforded by this structure,
then it should wrap it and provide its own allocator.

> > + This is not permitted to return an error. In the event of failure,
> > + ``netfs_prepare_write_failed()`` must be called.
> "This method is not permitted to return an error. Instead, in the event of
> failure, ..."

Seems superfluous, but okay.

(Btw, can you put a blank line before your "> <snipped>..." to make it easier
to go through your reply?)

Thanks,
David