On Wed, Dec 13, 2023 at 10:06:37AM +1100, Dave Chinner wrote: > On Wed, Dec 13, 2023 at 09:31:13AM +1100, NeilBrown wrote: > > On Wed, 13 Dec 2023, Dave Chinner wrote: > > > > > > What you are suggesting is that we now duplicate filehandle encoding > > > into every filesystem's statx() implementation. That's a bad > > > trade-off from a maintenance, testing and consistency POV because > > > now we end up with lots of individual, filehandle encoding > > > implementations in addition to the generic filehandle > > > infrastructure that we all have to test and validate. > > > > Not correct. We are suggesting an interface, not an implementation. > > Here you are proposing a suboptimal implementation, pointing out its > > weakness, and suggesting the has consequences for the interface > > proposal. Is that the strawman fallacy? > > No, you simply haven't followed deep enough into the rabbit hole to > understand Kent was suggesting potential implementation details to > address hot path performance concerns with filehandle encoding. > > > vfs_getattr_nosec could, after calling i_op->getattr, check if > > STATX_HANDLE is set in request_mask but not in ->result_mask. > > If so it could call exportfs_encode_fh() and handle the result. > > > > No filesystem need to be changed. > > Well, yes, it's pretty damn obvious that is exactly what I've been > advocating for here - if we are going to put filehandles in statx(), > then it must use the same infrastructure as name_to_handle_at(). > i.e. calling exportfs_encode_fh(EXPORT_FH_FID) to generate the > filehandle. > > The important discussion detail you've missed about > exportfs_encode_fh() is that it *requires* adding a new indirect > call (via export_ops->encode_fh) in the statx path to encode the > filehandle, and that's exactly what Kent was suggesting we can code > the implementation to avoid. > > Avoiding an indirect function call is an implementation detail, not > an interface design requirement. > > And the only way to avoid adding new indirect calls to encoding > filesystem specific filehandles is to implement the encoding in the > existing individual filesystem i_op->getattr methods. i.e. duplicate > the filehandle encoding in the statx path rather than use > exportfs_encode_fh()..... I was thinking along the lines of coming up with a common fh type for local filesystems (why exactly do we need 15?) and adding a volume ID to the VFS inode so this could live entirely in VS code for most filesystems, but that's an option too. Might be the best one, since btrfs and bcachefs actually do want a different fh type (btrfs: 64 bit subvol, 64 bit ino, bcachefs: 64 bit ino, 32 bit subvol, 32 bit generation), and we don't want to generate a bigger fh than necessary for when it's being consumed by a stacking filesystem that has to generate a new fh by concatanating something.
