Re: [PATCH] netfs: Update main API document

[Bagas Sanjaya <bagasdotme@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3564 bytes --]

On Wed, Apr 09, 2025 at 10:05:29AM +0100, David Howells wrote:
> 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.

OK.

> 
> > > +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.

That one sounds better to me.

> 
> > > +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?

I mean "However, this needs managing the inode (inode management)". Is it
correct to you?

> 
> > > -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.

Looks OK.

> 
> > > +   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?)

OK, thanks!

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]