From: Dave Chinner <david@fromorbit.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
"Darrick J. Wong" <darrick.wong@oracle.com>,
linux-xfs <linux-xfs@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
overlayfs <linux-unionfs@vger.kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] xfs: fix GPF in swapfile_activate of file from overlayfs
Date: Mon, 27 Aug 2018 08:59:52 +1000 [thread overview]
Message-ID: <20180826225952.GC2234@dastard> (raw)
In-Reply-To: <CAOQ4uxiHFmiq98OUJqzALm8hHC9azkXVE_bDdKXZLcq+kaCC3Q@mail.gmail.com>
On Sun, Aug 26, 2018 at 02:32:33PM +0300, Amir Goldstein wrote:
> On Sat, Aug 25, 2018 at 11:04 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sat, Aug 25, 2018 at 12:47 PM, Amir Goldstein <amir73il@gmail.com> 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().
What's file_real()?
I don't see it in 4.19-rc1 - is this a new vfs interface we're
expected to know about and use correctly without being told about it
and having no documentation explaining it's use to refer to?
> 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()
.... and this is how we found out that the light wasn't the end of
the tunnel, it was the oncoming train... :/
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2018-08-27 2:43 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-24 9:02 [PATCH] xfs: fix GPF in swapfile_activate of file from overlayfs Amir Goldstein
2018-08-24 23:39 ` Dave Chinner
2018-08-25 10:47 ` Amir Goldstein
2018-08-25 20:04 ` Miklos Szeredi
2018-08-26 11:32 ` Amir Goldstein
2018-08-26 22:59 ` Dave Chinner [this message]
2018-08-27 7:17 ` Amir Goldstein
2018-08-26 22:52 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180826225952.GC2234@dastard \
--to=david@fromorbit.com \
--cc=amir73il@gmail.com \
--cc=darrick.wong@oracle.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).