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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2025-06-09 at 09:33 -0400, Jan Harkes wrote:
> 
> On June 9, 2025 9:21:20 AM EDT, Jeff Layton <jlayton@kernel.org> 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.
> 

Yes. That would break coda as it's currently designed.  Actually, it
looks like DAX replaces i_mapping too, so we'd have to figure out
something there if we wanted to do this anyway.

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

Fair enough. I don't have any current plans to push for removing it,
but there is a maintenance burden here.

> 
> > > On June 9, 2025 9:00:31 AM EDT, Jan Kara <jack@suse.cz> 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@brown.name>
> > > > > > ---
> > > > > >  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@kernel.org>
> > > > 
> > > > 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
> > 

-- 
Jeff Layton <jlayton@kernel.org>