On Sat, Aug 25, 2018 at 11:04 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Sat, Aug 25, 2018 at 12:47 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > Actually, I believe the intention was that fs developers don't need to worry
> > about using file_inode() at all, because before the change we had:
> >
> > - file passed in to xfs f_op's and a_ops is either overlay file OR xfs file
> > - file_inode() of either overlay/xfs file in xfs context is always xfs inode
> > - file->f_path in xfs context, BTW, was overlay path and therefore,
> > XFS_IOC_OPEN_BY_HANDLE was slightly broken in overlayfs over xfs,
> > as were several other fs specific ioctls
> >
> > After stacked file operations change we should have the rules:
> >
> > 1. file passed in to xfs f_op's is always xfs file (*)
> > 2. file passed in to xfs a_ops is always xfs file (**)
> > 3. file_inode() of overlay file is an overlay inode
> >
> > (*) as explicit file argument or on iocb->ki_filp
> > (**) as explicit file argument or on ->vm_file
> >
> > I believe that swapfile leaking an overlay file into xfs was an oversight,
> > that is breaking rule #2.
>
> Correct.
>
> I believe the root cause is this
>
> /* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO */
> file->f_mapping = realfile->f_mapping;
>
> in ovl_open(). So lets start with removing that. That should fix any
> oopses related to this, but we'll have some other issues:
>
> 1) open(..., O_DIRECT) will return an error
>
> This is easy to fix: add ovl_file_aops with a dummy ovl_direct_IO()
> function that will never be called.
>
Nice. I pushed a patch to ovl-fixes branch [1].
> 2) swapon() will return an error
>
> First question that comes to mind: does anybody care? I wouldn't
> think swapfiles would be an important feature for overlayfs, but we
> did support them up till now, so removing support might cause a
> regression somewhere out there. Unfortunate...
>
I think we better introduce this "regression" and see if there are any real
world users out there. If there are, I have a WIP patch to make it work,
but it involves cloning an accountable file from real file - not a pretty sight.
3) readahead() will return an error and fadvise() will ignore request
I have patches [1] to fix those by familiarizing vfs with file_real().
4) I am afraid we may end up with more vfs hacks -
right off the top of my grep f_mapping fs/*.c:
- FIBMAP
- sync_file_range()
But wasn't able to demonstrate a problem with those yet.
I am running [1] through xfstests now and if it looks fine, I'll post
the patches.
Thanks,
Amir.
[1] https://github.com/amir73il/linux/commits/ovl-fixes
