Re: [PATCH v4 1/3] libfs: Add directory operations for stable offsets

[Jeff Layton <jlayton@redhat.com>]

On Tue, 2023-06-27 at 14:04 +0000, Chuck Lever III wrote:
> 
> > On Jun 27, 2023, at 4:52 AM, Christian Brauner <brauner@kernel.org> wrote:
> > 
> > On Mon, Jun 26, 2023 at 11:44:15PM -0700, Christoph Hellwig wrote:
> > > > + * @dir: parent directory to be initialized
> > > > + *
> > > > + */
> > > > +void stable_offset_init(struct inode *dir)
> > > > +{
> > > > + xa_init_flags(&dir->i_doff_map, XA_FLAGS_ALLOC1);
> > > > + dir->i_next_offset = 0;
> > > > +}
> > > > +EXPORT_SYMBOL(stable_offset_init);
> > > 
> > > If this is exported I'd much prefer a EXPORT_SYMBOL_GPL.  But the only
> > > user so far is shmfs, which can't be modular anyway, so I think we can
> > > drop the exports.
> > > 
> > > > --- a/include/linux/dcache.h
> > > > +++ b/include/linux/dcache.h
> > > > @@ -96,6 +96,7 @@ struct dentry {
> > > > struct super_block *d_sb; /* The root of the dentry tree */
> > > > unsigned long d_time; /* used by d_revalidate */
> > > > void *d_fsdata; /* fs-specific data */
> > > > + u32 d_offset; /* directory offset in parent */
> > > > 
> > > > union {
> > > > struct list_head d_lru; /* LRU list */
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index 133f0640fb24..3fc2c04ed8ff 100644
> > > > --- a/include/linux/fs.h
> > > > +++ b/include/linux/fs.h
> > > > @@ -719,6 +719,10 @@ struct inode {
> > > > #endif
> > > > 
> > > > void *i_private; /* fs or device private pointer */
> > > > +
> > > > + /* simplefs stable directory offset tracking */
> > > > + struct xarray i_doff_map;
> > > > + u32 i_next_offset;
> > > 
> > > We can't just increase the size of the dentry and inode for everyone
> > > for something that doesn't make any sense for normal file systems.
> > > This needs to move out into structures allocated by the file system
> > > and embedded into or used as the private dentry/inode data.
> > 
> > I agree. I prefer if this could be done on a per filesystem basis as
> > well. Especially since, this is currently only useful for a single
> > filesystem.
> > 
> > We've tried to be very conservative in increasing inode and dentry size
> > and we should continue with that.
> 
> I had thought we were going to adopt the stable offset mechanism
> in more than just shmemfs. That's why we're putting it in libfs.c
> after all. So this was not going to be "just for shmemfs" in the
> long run.
> 
> That said, I can move the stable-offset-related inode fields back
> into the shmemfs private inode struct, as it was in v2 of this
> series.
> 

I too think that would be best. The libfs helpers will probably need to
take an extra pointer to the relevant fields in the inode and dentry,
but that's not too hard to do.

> For d_offset, I was (ab)using the d_fsdata field by casting the
> offset value as a pointer. That's kind of ugly. Any suggestions?
> 
> 

Absolutely nothing wrong with interpreting that as an integer. It's a
void * which means that anything that wants to dereference it better
know what it is ahead of time anyway.
-- 
Jeff Layton <jlayton@redhat.com>