Re: [PATCH 3/5] coda: use iterate_dir() in coda_readdir()

[Jan Harkes]

On June 9, 2025 9:21:20 AM EDT, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>On Mon, 2025-06-09 at 09:12 -0400, Jan Harkes wrote:
>> There are definitely still users of Coda at CMU, I don't track who else uses it, but it cannot be too many for sure.
>> 
>> At this point it mostly keeps you honest about little locking details in the vfs. There are some tricky details in how the inode mappings are accessed and such. I think that  is helpful for overlay and user filesystems like fuse, overlayfs, etc. but Coda is quite small so it is easy to reason about how it uses these features.
>> 
>> 
>
>The reason I ask is that it's one of the places that we often have to
>do odd fixups like this when making changes to core VFS APIs. It's also
>not seen any non-collateral changes since 2021. I'm just wondering
>whether it's worth it to keep coda in-tree for so few users.

I have no problem maintaining an external module. I already do that to both make development easier and to support building a custom module in case some distro didn't ship with a prebuilt Coda kernel module.

>IIRC, it's also the only fs in the kernel that changes its
>inode->i_mapping pointer after inode instantiation. If not for coda, we
>could probably replace the i_mapping pointer with some macro wizardry
>and shrink struct inode by 8 bytes.

And that would be why an out-of-tree solution will not work in the long run. A small change like that to shave 8 bytes off the inode structure is a fairly easy call to make when there are no in-tree filesystems that use it anymore.

Ultimately it is up to you guys to decide if the extra burden on VFS maintenance is worth it.

Jan

>> On June 9, 2025 9:00:31 AM EDT, Jan Kara <jack@xxxxxxx> wrote:
>> > On Mon 09-06-25 08:17:15, Jeff Layton wrote:
>> > > On Mon, 2025-06-09 at 09:09 +1000, NeilBrown wrote:
>> > > > The code in coda_readdir() is nearly identical to iterate_dir().
>> > > > Differences are:
>> > > >  - iterate_dir() is killable
>> > > >  - iterate_dir() adds permission checking and accessing notifications
>> > > > 
>> > > > I believe these are not harmful for coda so it is best to use
>> > > > iterate_dir() directly.  This will allow locking changes without
>> > > > touching the code in coda.
>> > > > 
>> > > > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
>> > > > ---
>> > > >  fs/coda/dir.c | 12 ++----------
>> > > >  1 file changed, 2 insertions(+), 10 deletions(-)
>> > > > 
>> > > > diff --git a/fs/coda/dir.c b/fs/coda/dir.c
>> > > > index ab69d8f0cec2..ca9990017265 100644
>> > > > --- a/fs/coda/dir.c
>> > > > +++ b/fs/coda/dir.c
>> > > > @@ -429,17 +429,9 @@ static int coda_readdir(struct file *coda_file, struct dir_context *ctx)
>> > > >  	cfi = coda_ftoc(coda_file);
>> > > >  	host_file = cfi->cfi_container;
>> > > >  
>> > > > -	if (host_file->f_op->iterate_shared) {
>> > > > -		struct inode *host_inode = file_inode(host_file);
>> > > > -		ret = -ENOENT;
>> > > > -		if (!IS_DEADDIR(host_inode)) {
>> > > > -			inode_lock_shared(host_inode);
>> > > > -			ret = host_file->f_op->iterate_shared(host_file, ctx);
>> > > > -			file_accessed(host_file);
>> > > > -			inode_unlock_shared(host_inode);
>> > > > -		}
>> > > > +	ret = iterate_dir(host_file, ctx);
>> > > > +	if (ret != -ENOTDIR)
>> > > >  		return ret;
>> > > > -	}
>> > > >  	/* Venus: we must read Venus dirents from a file */
>> > > >  	return coda_venus_readdir(coda_file, ctx);
>> > > >  }
>> > > 
>> > > 
>> > > Is it already time for my annual ask of "Who the heck is using coda
>> > > these days?" Anyway, this patch looks fine to me.
>> > > 
>> > > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> > 
>> > Send a patch proposing deprecating it and we might learn that :) Searching
>> > the web seems to suggest it is indeed pretty close to dead.
>> > 
>> > 								Honza
>