[GIT PULL] dax fixes for 4.20-rc6

[GIT PULL] dax fixes for 4.20-rc6

[Williams, Dan J]

Hi Linus, please pull from:

  git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6

...to receive the last of the known regression fixes and fallout from
the Xarray conversion of the filesystem-dax implementation. On the path
to debugging why the dax memory-failure injection test started failing
after the Xarray conversion a couple more fixes for the
dax_lock_mapping_entry(), now called dax_lock_page(), surfaced. Those
plus the bug that started the hunt are now addressed. These patches
have appeared in a -next release with no issues reported.

Note the touches to mm/memory-failure.c are just the conversion to the
new function signature for dax_lock_page().

---

The following changes since commit 2e6e902d185027f8e3cb8b7305238f7e35d6a436:

  Linux 4.20-rc4 (2018-11-25 14:19:31 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6

for you to fetch changes up to 27359fd6e5f3c5db8fe544b63238b6170e8806d8:

  dax: Fix unlock mismatch with updated API (2018-12-04 21:32:00 -0800)

----------------------------------------------------------------
dax fixes 4.20-rc6

* Fix the Xarray conversion of fsdax to properly handle
dax_lock_mapping_entry() in the presense of pmd entries.

* Fix inode destruction racing a new lock request.

----------------------------------------------------------------
Matthew Wilcox (3):
      dax: Check page->mapping isn't NULL
      dax: Don't access a freed inode
      dax: Fix unlock mismatch with updated API

 fs/dax.c            | 55 ++++++++++++++++++++++++++++++++++++-----------------
 include/linux/dax.h | 14 ++++++++------
 mm/memory-failure.c |  6 ++++--
 3 files changed, 50 insertions(+), 25 deletions(-)

Re: [GIT PULL] dax fixes for 4.20-rc6

[Linus Torvalds]

On Sat, Dec 8, 2018 at 10:26 PM Williams, Dan J
<dan.j.williams@intel.com> wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6

What's going on with the odd non-exclusive exclusive wait?

        prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
        ...
        /*
         * Entry lock waits are exclusive. Wake up the next waiter since
         * we aren't sure we will acquire the entry lock and thus wake
         * the next waiter up on unlock.
         */
        if (waitqueue_active(wq))
                __wake_up(wq, TASK_NORMAL, 1, &ewait.key);

that seems to make little or no sense.

Why isn't that prepare_to_wait_exclusive() just a regular
prepare_to_wait(), and then the whole "let's wake up anybody else" can
be removed?

I've pulled it, but am awaiting explanation of what looks like some
pretty crazy code. I *suspect* it's a copy-and-paste situation where
you took the exclusive wait from somewhere else.

                Linus

Re: [GIT PULL] dax fixes for 4.20-rc6

[Dan Williams]

[ add Willy and Jan ]

On Sun, Dec 9, 2018 at 10:02 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Dec 8, 2018 at 10:26 PM Williams, Dan J
> <dan.j.williams@intel.com> wrote:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6
>
> What's going on with the odd non-exclusive exclusive wait?
>
>         prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
>         ...
>         /*
>          * Entry lock waits are exclusive. Wake up the next waiter since
>          * we aren't sure we will acquire the entry lock and thus wake
>          * the next waiter up on unlock.
>          */
>         if (waitqueue_active(wq))
>                 __wake_up(wq, TASK_NORMAL, 1, &ewait.key);
>
> that seems to make little or no sense.
>
> Why isn't that prepare_to_wait_exclusive() just a regular
> prepare_to_wait(), and then the whole "let's wake up anybody else" can
> be removed?
>
> I've pulled it, but am awaiting explanation of what looks like some
> pretty crazy code. I *suspect* it's a copy-and-paste situation where
> you took the exclusive wait from somewhere else.

Yes, I believe that's true. In the other instances of waiting for an
entry to be in unlocked there is a guarantee that the waiter will
attain the lock and perform an unlock + wakeup. In the dax_lock_page()
path there is the possibility that the inode dies before the lock is
attained and a subsequent unlock sequence is not guaranteed. So, I
believe the intent, Willy correct me if I am wrong, was to keep all
waits "exclusive" for some sense of symmetry, but this one can and
should be a non-exclusive wait.

I can send a cleanup, do you want one immediately, or is post -rc6 ok?

Re: [GIT PULL] dax fixes for 4.20-rc6

[Linus Torvalds]

On Sun, Dec 9, 2018 at 10:27 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> I can send a cleanup, do you want one immediately, or is post -rc6 ok?

post-rc6 is fine, I'd rather have the patch tested anyway,

                 Linus

Re: [GIT PULL] dax fixes for 4.20-rc6

[Matthew Wilcox]

On Sun, Dec 09, 2018 at 10:26:54AM -0800, Dan Williams wrote:
> [ add Willy and Jan ]
> 
> On Sun, Dec 9, 2018 at 10:02 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sat, Dec 8, 2018 at 10:26 PM Williams, Dan J
> > <dan.j.williams@intel.com> wrote:
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6
> >
> > What's going on with the odd non-exclusive exclusive wait?
> >
> >         prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
> >         ...
> >         /*
> >          * Entry lock waits are exclusive. Wake up the next waiter since
> >          * we aren't sure we will acquire the entry lock and thus wake
> >          * the next waiter up on unlock.
> >          */
> >         if (waitqueue_active(wq))
> >                 __wake_up(wq, TASK_NORMAL, 1, &ewait.key);
> >
> > that seems to make little or no sense.
> >
> > Why isn't that prepare_to_wait_exclusive() just a regular
> > prepare_to_wait(), and then the whole "let's wake up anybody else" can
> > be removed?
> >
> > I've pulled it, but am awaiting explanation of what looks like some
> > pretty crazy code. I *suspect* it's a copy-and-paste situation where
> > you took the exclusive wait from somewhere else.
> 
> Yes, I believe that's true. In the other instances of waiting for an
> entry to be in unlocked there is a guarantee that the waiter will
> attain the lock and perform an unlock + wakeup. In the dax_lock_page()
> path there is the possibility that the inode dies before the lock is
> attained and a subsequent unlock sequence is not guaranteed. So, I
> believe the intent, Willy correct me if I am wrong, was to keep all
> waits "exclusive" for some sense of symmetry, but this one can and
> should be a non-exclusive wait.

I did indeed just copy it from elsewhere.  As I said at the time,

"This is the best I've come up with.  Could we do better by not using
the _exclusive form of prepare_to_wait()?  I'm not familiar with all the
things that need to be considered when using this family of interfaces."

but nobody suggested what the right way to use these interfaces was.

https://lists.01.org/pipermail/linux-nvdimm/2018-November/018892.html

Re: [GIT PULL] dax fixes for 4.20-rc6

[Jan Kara]

On Sun 09-12-18 10:26:54, Dan Williams wrote:
> [ add Willy and Jan ]
> 
> On Sun, Dec 9, 2018 at 10:02 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Sat, Dec 8, 2018 at 10:26 PM Williams, Dan J
> > <dan.j.williams@intel.com> wrote:
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6
> >
> > What's going on with the odd non-exclusive exclusive wait?
> >
> >         prepare_to_wait_exclusive(wq, &ewait.wait, TASK_UNINTERRUPTIBLE);
> >         ...
> >         /*
> >          * Entry lock waits are exclusive. Wake up the next waiter since
> >          * we aren't sure we will acquire the entry lock and thus wake
> >          * the next waiter up on unlock.
> >          */
> >         if (waitqueue_active(wq))
> >                 __wake_up(wq, TASK_NORMAL, 1, &ewait.key);
> >
> > that seems to make little or no sense.
> >
> > Why isn't that prepare_to_wait_exclusive() just a regular
> > prepare_to_wait(), and then the whole "let's wake up anybody else" can
> > be removed?
> >
> > I've pulled it, but am awaiting explanation of what looks like some
> > pretty crazy code. I *suspect* it's a copy-and-paste situation where
> > you took the exclusive wait from somewhere else.
> 
> Yes, I believe that's true. In the other instances of waiting for an
> entry to be in unlocked there is a guarantee that the waiter will
> attain the lock and perform an unlock + wakeup. In the dax_lock_page()
> path there is the possibility that the inode dies before the lock is
> attained and a subsequent unlock sequence is not guaranteed. So, I
> believe the intent, Willy correct me if I am wrong, was to keep all
> waits "exclusive" for some sense of symmetry, but this one can and
> should be a non-exclusive wait.

Agreed. I didn't realize this when reviewing Matthew's patch and
misunderstood his comment to this end.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [GIT PULL] dax fixes for 4.20-rc6

[pr-tracker-bot]

The pull request you sent on Sun, 9 Dec 2018 06:26:39 +0000:

> git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm tags/dax-fixes-4.20-rc6

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/fa82dcbf2aed65dc3ea78eaca9ea56fd926b2b10

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker