On Wed, Apr 09, 2025 at 10:05:29AM +0100, David Howells wrote:
> Bagas Sanjaya <bagasdotme@xxxxxxxxx> 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:
signature.asc
Description: PGP signature
