On Tue, Aug 22, 2023 at 2:03 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Tue, 22 Aug 2023 at 12:18, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Mon, Aug 21, 2023 at 6:27 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > Getting back to this. > > > Did you mean something like that? (only compile tested) > > > > > > https://github.com/amir73il/linux/commits/backing_fs > > > > > > If yes, then I wonder: > > > 1. Is the difference between FUSE_IOCB_MASK and OVL_IOCB_MASK > > > (i.e. the APPEND flag) intentional? > > Setting IOCB_APPEND on the backing file doesn't make a difference as > long as the backing file is not modified during the write. > > In overlayfs the case of the backing file being modified is not > defined, so I guess that's the reason to omit it. However I don't see > a problem with setting it on the backing file either, the file > size/position is synchronized after the write, so nothing bad should > happen if the backing file was modified. > > > > 2. What would be the right way to do ovl_copyattr() on io completion? > > > Pass another completion handler to read/write helpers? > > > This seems a bit ugly. Do you have a nicer idea? > > > > > Ugh, I missed that little detail. I don't have a better idea than to > use a callback function. > > > > > Hmm. Looking closer, ovl_copyattr() in ovl_aio_cleanup_handler() > > seems a bit racy as it is not done under inode_lock(). > > > > I wonder if it is enough to fix that by adding the lock or if we need > > to resort to a more complicated scheme like FUSE_I_SIZE_UNSTABLE > > for overlayfs aio? > > Quite recently rename didn't take inode lock on source, so > ovl_aio_cleanup_handler() wasn't the only unlocked instance. > > I don't see a strong reason to always lock the inode before > ovl_copyattr(), but I could be wrong. > IDK, ovl_copyattr() looks like a textbook example of a race if not protected by something because it reads a bunch of stuff from realinode and then writes a bunch of stuff to inode. Anyway, I guess it wouldn't hurt to wrap it with inode_lock() in the ovl completion callback. Thanks, Amir.
