On Wed, Nov 1, 2023 at 1:32 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Tue, 31 Oct 2023 at 18:44, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > In that case, we would be able to "attach" the fuse_backing object > > to fuse_inode on CREATE response. If we end up with a backing map > > conflict at this point, we can return -EBUSY error to the user and forget > > the inode, but the server would have no easy feedback on its mistake. > > Also, -EBUSY to user would be confusing if user did not request O_EXCL. > > I think -EIO is more appropriate. Server is broken, WARN_ON_ONCE > could also be used to indicate that. > WARN_ON is a kernel assertion - we should not use it for user possible wrong inputs. we can use pr_warn_ratelimited(). According to your suggestion below that FOPEN_PASSTHROUGH is an advice flag, we can simply ignore the conflicting passthrough request and keep passthrough on the existing backing file without any feedback to the user or server. I can live with that. > > Do you consider the described "atomic_open conflict" case an API problem? > > > > It is true that with v14 patches that do not enforce backing inode conflicts, > > the attribute coherency model that I proposed may result in attribute > > cache thrashing if the backing inode of a fuse inode is ambiguous. > > > > Do you have an idea how to handle this case elegantly? > > Let me add some perspective. > > Currently we have FOPEN_DIRECT_IO that disables caching. My feeling > when dealing with this interface is that it was a mistake to make this > a property of the open file. It should insted be a property of the > inode and all open file instances should respect this property > equally. It makes no sense to have one file do cached reads while the > other is doing direct writes, etc. Also it should be possible to turn > this on or off for all open file instances at any time. > > Passthrough is similar, I think. At any point in time all I/O should either be > > - cached > - direct > - passthrough to a specific backing file > > Allowing these to be mixed leads to confusing semantics, especially > cached and non-cached > > OTOH allowing passthrough to be switched on at any point in time > presents challenges. If passthrough is turned on for an inode that > didn't have it previously means that the backing file needs to be set > up before the actual I/O. So it's definitely more complex than just > setting up the backing at open. But I feel that longer term we'll end > up with a better interface. > > For the first version we can just bypass this whole mess, and say that > passthrough can only be turned on for the first open. I.e. if there > are already open instances and passthrough has not yet been set up, > then FOPEN_PASSTHROUGH will be ignored. Similarly if it has already > been set up, then the lack of FOPEN_PASSTHROUGH is also ignored. > > Hmm? Sounds like a small change, which will work fine with the example server as well as with my in-house server requirement for regular files. However, I can imagine users wanting to do passthrough for read-only fd without doing passthrough for read-write fd, for example, if data is never manipulated by the server, but it is inspected on write. I wonder if this model fits the existing use of Android applications with the v12 patches. I have one problem with my in-house server and passthough of readdir. readdir passthrough cannot do readdirplus to populate inode cache. For readdir of a large, read-mostly directory, cached readdir is preferred because of readdirplus. For a large, write-often directory, when only readdir is needed, readdir passthough is preferred. No one size fits all here. My in-house server chooses whether to do readdir passthrough based on userspace hint regarding readdir vs. readdirplus. Since directory has no "write modes" and since FOPEN_CACHE_DIR is going to stay per-fd, I do not really see a problem with allowing readdir passthrough and cached/uncached readdir on the same inode. Do you see a problem with that? I don't mind dropping the last readdir passthrough patch for the first version, if you want to take more time to think this over. I'd just like to know that there is a path forward to make conditional passthrough per fd possible in future versions. BTW, the FUSE BPF patches that map a backing inode to fuse inode allow fallback to server depending on BPF program result. Thoughts? Thanks, Amir.
