linux-kernel - Re: [PATCH] netfs: Update main API document

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_ZHoCgi2BY5lVjN@archie.me>
Date: Wed, 9 Apr 2025 17:10:40 +0700
From: Bagas Sanjaya <bagasdotme@...il.com>
To: David Howells <dhowells@...hat.com>
Cc: Christian Brauner <brauner@...nel.org>,
	Paulo Alcantara <pc@...guebit.com>,
	Jeff Layton <jlayton@...nel.org>,
	Viacheslav Dubeyko <slava@...eyko.com>,
	Alex Markuze <amarkuze@...hat.com>, Timothy Day <timday@...zon.com>,
	Jonathan Corbet <corbet@....net>, netfs@...ts.linux.dev,
	linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netfs: Update main API document

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)