| Message ID | 6be0835bf256755375f3b5822bf70d74538d1d6e.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] dax fixes for 4.20-rc6 | expand |
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
[ 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?
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
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!
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
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
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(-)