Re: [PATCH] netfs: Update main API document

[Bagas Sanjaya]

On Wed, Apr 09, 2025 at 02:24:23PM +0100, David Howells wrote:
> Bagas Sanjaya <bagasdotme@xxxxxxxxx> 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: signature.asc
Description: PGP signature