Messages in this thread | | | Date | Mon, 10 Dec 2018 13:21:08 +0100 | From | Jan Kara <> | Subject | Re: [GIT PULL] dax fixes for 4.20-rc6 |
| |
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
| |