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

[Jeff Layton <jlayton@kernel.org>]

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.

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.


> 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>