On Thu, Jun 19, 2025 at 10:29 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Thu, Jun 19, 2025 at 9:50 PM Paul Lawrence <paullawrence@xxxxxxxxxx> wrote: > > > > Hi Amir, > > > > Thank you for your detailed reply. My intent with this patch was to see if there > > was interest (a definite yes) and to see what path would best get us > > to our common > > goal. > > > > I'm thinking the best approach is to start with your ops_mask API. In > > fact, that solves > > the biggest single problem with my future patch set, which was that it > > was going to be > > huge and not realistically divisible, since you need everything for > > directory passthrough > > to work without the mask. Your way allows us to proceed in nice > > logical steps, which is > > much, much better. Thank you for that suggestion. > > > > So my follow-up question is: What can I do to help get the > > foundational patches you > > wrote upstreamed? > > Well you can always take them and re-shape them and post them > to see what the maintainers think and address the feedback. > But I can try to beat them to shape myself to at least post v1. > > > > > In the meantime, a few thoughts on your comments. (Note that one of > > the beauties of > > your suggestion is that we don't need to agree on any of this to get > > started - we can > > discuss them in detail when we get to the specific ops that require them.) > > > > 1) Yes, let's use backing_id. I won't mention that again. > > > > 2) The backing path per dentry comes from the way dentry_open works. > > If we are going to > > attach a file to a lookup, we have to put something into the > > fuse_dentry or the fuse_inode. > > There is already fuse_backing *fb in fuse_inode. > I don't understand why anything else is needed for implementing > passthrough dir ops. > > > This makes more sense once you see points 3 & 4 below - without them, > > we have an open > > file, so why not just use it? > > We need to make the code simple enough. > Not add things that are not needed. > > > > > 3) A cute idea that we had that seems to work is to allow negative > > dentries as backing > > dentries. It appears to work well - for instance, a create first looks > > up the (negative) dentry > > then creates the file into that dentry. If the lookup puts a negative > > dentry as the backing > > file, we can now just use vfs_create to create the backing file. > > > > That sounds like trouble. > Overalyfs effectively implements passthrough dir ops. > It doesn't keep negative backing dentries, so I doubt that this is needed. > > > This means that only FUSE_LOOKUP and (I think) FUSE_READDIRPLUS need to have > > the ability to accept backing_ids. I think is is both more elegant > > conceptually, simpler to > > code in the kernel *and* simpler to use in the daemon. > > > > 4) Having to open a file for it to be passed into a lookup is > > problematic. Imagine > > readdirplus on a large folder. We would need to open every single > > backing file, and it > > would stay open until the dentry was removed from the cache. > > We are talking about opening a O_PATH fd at lookup. > The daemon does not need to keep this O_PATH fd open, > although production daemons today (e.g. virtiofsd) anyway > keep an open O_PATH fd per fuse inode in cache. > > Maybe it is a problem, but I am not convinced that it is, so > maybe I need more details about what problems this is causing. > If you are going to pin the backing inode/dentry to cache, then most > of the memory resources are already taken, the extra file does not add > much memory and it is currently not accounted for in any process. > > > > > Both of these suggest that rather than just passing a backing_id to FUSE_LOOKUP > > and FUSE_READDIRPLUS we should be able to pass a backing_id and a relative path. > > This is where the idea of putting the backing path into the fuse > > dentry comes from. > > > > Sorry this is too much hand waving. > I still don't understand what problem attaching a backing path to every dentry > solves. You will have to walk me through exactly what the problem is with > having the backing file/path attached to the inode. > > > I don't *think* this creates any security issues, so long as the > > relative path is traversed > > in the context of the daemon. (We might want to ban '..' and traverses > > over file systems.) > > Sorry you lost me. > I do not understand the idea of backing_id and a relative path. > passthrough of READDIRPLUS is complicated. > If you have an idea I need to see a very detailed plan. > > > Again, these are details we can debate when the patches are ready for > > discussion. > > > > But again, let's start with your patch set. What are the next steps in > > taking it upstream? > > And which are the next ops you would like to see implemented? I would > > be happy to take > > a stab at one or two. > > > > I can post patches for passthrough getxattr/listxattr, those are pretty > simple, Spoke up too soon. passthrough of getxattr is only allegedly simple - that's before realizing the complexity of passthrough of get_acl (see ovl_get_acl()). > but I am not sure if they have merit on their own without > passthrough of getattr, which is more complicated. > > Also I am not sure that implementing passthrough of some inode ops > has merit without being able to setup passthrough at lookup time. > > I will see if I can find time to post a POC of basic passthrough of inode > ops and setup of backing id at lookup time. > I did not get far enough to post a POC, but I did get far enough to test inode ops passthrough. This branch [1] has patches that implement passthrough for readdir and getattr/statx. I mostly wanted to demonstrate that you do not need to invent any new infra to support passthrough of inode/dir ops - all the infra is already there just waiting for a richer API to setup inode ops passthrough at lookup time and implement the different passthrough methods. I hope that this is clear? I ran into an ABI compatibility issue with extending the fuse_entry_out lookup result, which is also being used in struct fuse_direntplus, so for the demo, I worked around this problem by setting up the iops passthrough on first GETATTR. The ABI compatibility (with existing binaries) is solvable but requires more work. In this demo passthrough_fs [2], when run with arguments --readdirpassthrough --nocache the first GETATTR on every dir/file sets up a permanent backing inode for the fuse inode and all the following GETATTR calls are passed through on this inode (until drop_caches). Readdir is also passed through, but because passthrough of LOOKUP is not implemented, it is not a clear win to use readdir/getattr passthrough in comparison to readdirplus. Hence, there is no justification to post these patches on their own. But you can use them as a basis for your work. If you have any questions, don't hesitate to ask me. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fuse_passthrough_iops/ [2] https://github.com/amir73il/libfuse/commits/fuse_passthrough_iops/
