Re: [PATCH] netfs: Update main API document

[David Howells <dhowells@redhat.com>]

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