Re: [PATCH] netfs: Update main API document

Bagas Sanjaya
2025-04-09T13:54:54Z

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

On Wed, Apr 09, 2025 at 02:24:23PM +0100, David Howells wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> > > > > +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.
>
> I think I like this better:
>
> Further, if one or more contiguous cache-read subrequests fail, the
> library will pass them to the filesystem to perform instead,
> renegotiating and retiling them as necessary to fit with the
> filesystem's parameters rather than those of the cache.

I prefer that above too as it is more explicit.

>
> > > > > +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?
>
> Um. "However, this needs managing the inode (inode management)" isn't valid
> English and "(inode management)" is superfluous with "managing the inode" also
> in the sentence.
>
> How about:
>
> 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 pinning
> needs careful management. To manage the pinning, the following sequence
> occurs:
>
> 1) An inode state flag ``I_PINNING_NETFS_WB`` is set by netfslib when the
> pinning begins (when a folio is dirtied, for example) if the cache is
> active to stop the cache structures from being discarded and the cache
> space from being culled. This also prevents re-getting of cache resources
> if the flag is already set.
>
> 2) This flag then cleared inside the inode lock during inode writeback in the
> VM - and the fact that it was set is transferred to ``->unpinned_netfs_wb``
> in ``struct writeback_control``.
>
> 3) If ``->unpinned_netfs_wb`` is now set, the write_inode procedure is forced.
>
> 4) The filesystem's ``->write_inode()`` function is invoked to do the cleanup.
>
> 5) The filesystem invokes netfs to do its cleanup.
>
> To do the cleanup, netfslib provides a function to do the resource unpinning::
>
> int netfs_unpin_writeback(struct inode *inode, struct writeback_control *wbc);
>
> If the filesystem doesn't need to do anything else, this may be set as a its
> ``.write_inode`` method.
>
> Further, if an inode is deleted, the filesystem's write_inode method may not
> get called, so::
>
> void netfs_clear_inode_writeback(struct inode *inode, const void *aux);
>
> must be called from ``->evict_inode()`` *before* ``clear_inode()`` is called.
>
>
> instead?

Oh, that's what you mean. I'm leaning toward that.

Thanks.

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

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