On Thu, Sep 21, 2023 at 6:51 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Wed, Sep 13, 2023 at 2:24 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Wed, Sep 13, 2023 at 11:29 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > On Tue, Sep 12, 2023 at 09:54:08PM +0300, Amir Goldstein wrote: > > > > Overlayfs stores its files data in backing files on other filesystems. > > > > > > > > Factor out some common helpers to perform io to backing files, that will > > > > later be reused by fuse passthrough code. > > > > > > > > Suggested-by: Miklos Szeredi <miklos@xxxxxxxxxx> > > > > Link: https://lore.kernel.org/r/CAJfpeguhmZbjP3JLqtUy0AdWaHOkAPWeP827BBWwRFEAUgnUcQ@xxxxxxxxxxxxxx > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > > > --- > > > > > > > > Miklos, > > > > > > > > This is the re-factoring that you suggested in the FUSE passthrough > > > > patches discussion linked above. > > > > > > > > This patch is based on the overlayfs prep patch set I just posted [1]. > > > > > > > > Although overlayfs currently is the only user of these backing file > > > > helpers, I am sending this patch to a wider audience in case other > > > > filesystem developers want to comment on the abstraction. > > > > > > > > We could perhaps later considering moving backing_file_open() helper > > > > and related code to backing_file.c. > > > > > > > > In any case, if there are no objections, I plan to queue this work > > > > for 6.7 via the overlayfs tree. > > > > > > > > Thanks, > > > > Amir. > > > > > > > > [1] https://lore.kernel.org/linux-unionfs/20230912173653.3317828-1-amir73il@xxxxxxxxx/ > > > > > > > > > > > > MAINTAINERS | 2 + > > > > fs/Kconfig | 4 + > > > > fs/Makefile | 1 + > > > > fs/backing_file.c | 160 +++++++++++++++++++++++++++++++++++ > > > > > > I'm sorry but I'm missing mountains of context. > > > How is that related to the backing file stuff exactly? > > > The backing file stuff has this unpleasant > > > > > > file->f_inode == real_inode != file->f_path->dentry->d_inode > > > > > > that we all agree is something we really don't like. Is FUSE trying to > > > do the same thing and build an read_iter/write_iter abstraction around > > > it? I really really hope that's not the case. > > > > That is not the case. > > The commonality between FUSE passthrough and overlayfs is that > > a "virtual" file (i.e. ovl/fuse), which has no backing blockdev of its own > > "forwards" the io requests to a backing file on another filesystem. > > > > The name "backing file" is therefore a pretty accurate description > > for both cases. HOWEVER, FUSE does not need to use the > > backing_file struct to hold an alternative path, so FUSE backing files > > do not have FMODE_BACKING, same as cachefiles uses backing > > files, but does not use the FMODE_BACKING/file_backing struct. > > > > Yes, it's a bit of a naming mess. > > I don't have any good ideas on how to do better naming. > > Ideally, we will get rid of struct backing_file, so we won't need > > to care about the confusing names... > > > > Alas, my explanation about FUSE passthrough backing files > turned out to be inaccurate. > > FUSE IO passthrough to backing file is very similar to overlayfs > IO "passthrough" to lower/upper backing file. > > Which creates the same problem w.r.t mmap'ing the FUSE file > to the underlying backing file inode. > That problem in overlayfs caused the inception of the fake path file > now embedded in the backing_file object. > So yes, FUSE is trying to do the same thing. > > However, the helpers in this patch are not dealing with the fake path > aspect of those backing files specifically. > They are common code for a "stacked fs" (i.e. s_stack_depth > 0) > which delegates IO to files on the underlying fs. > Funny story: following my clarification of the terminology above I was going to rename the common helpers and I wanted to create include/linux/fs_stack.h, but what do you know, it already exists. It was created in 2006 to have common helpers for eCryptfs and *Unionfs* (not overlayfs) and indeed, today it has a few helpers which only ecryptfs uses. Not only that, but it appears that the copy_attr_size helper does things that ovl does not do - need to look into that and copy_attr_all does not deal with uid translations and atomicity (which I just added to the respective ovl helper). So I am going to shake the dust from fs/stack.c and merge those diverged helpers for starters and then go on to adding the new common helpers for read/write/splice iter and code for handling the backing_file (or stacked_file) fake path anomaly. > I will give it some thought on how we can at least narrow the amount > of filesystem code that could be exposed to those fake path files. > The read/write/splice iterators do not really need to operate of fake path > files, so this is something that I can try to avoid. > I am not sure but there may be another problem related to mmap and the backing_file because ovl_mmap() of writable mapping does not keep an elevated mount writer count on the upper fs mount. The easy solution is perhaps for overlayfs mount to keep the elevated mount writer count on the upper fs mount. Not sure if this is really something to worry about. I will need to take a closer look. Thanks, Amir.
