On Mon 19-04-21 17:36:36, Vivek Goyal wrote:
> I am seeing missed wakeups which ultimately lead to a deadlock when I am
> using virtiofs with DAX enabled and running "make -j". I had to mount
> virtiofs as rootfs and also reduce to dax window size to 256M to reproduce
> the problem consistently.
>
> So here is the problem. put_unlocked_entry() wakes up waiters only
> if entry is not null as well as !dax_is_conflict(entry). But if I
> call multiple instances of invalidate_inode_pages2() in parallel,
> then I can run into a situation where there are waiters on
> this index but nobody will wait these.
>
> invalidate_inode_pages2()
> invalidate_inode_pages2_range()
> invalidate_exceptional_entry2()
> dax_invalidate_mapping_entry_sync()
> __dax_invalidate_entry() {
> xas_lock_irq(&xas);
> entry = get_unlocked_entry(&xas, 0);
> ...
> ...
> dax_disassociate_entry(entry, mapping, trunc);
> xas_store(&xas, NULL);
> ...
> ...
> put_unlocked_entry(&xas, entry);
> xas_unlock_irq(&xas);
> }
>
> Say a fault in in progress and it has locked entry at offset say "0x1c".
> Now say three instances of invalidate_inode_pages2() are in progress
> (A, B, C) and they all try to invalidate entry at offset "0x1c". Given
> dax entry is locked, all tree instances A, B, C will wait in wait queue.
>
> When dax fault finishes, say A is woken up. It will store NULL entry
> at index "0x1c" and wake up B. When B comes along it will find "entry=0"
> at page offset 0x1c and it will call put_unlocked_entry(&xas, 0). And
> this means put_unlocked_entry() will not wake up next waiter, given
> the current code. And that means C continues to wait and is not woken
> up.
>
> This patch fixes the issue by waking up all waiters when a dax entry
> has been invalidated. This seems to fix the deadlock I am facing
> and I can make forward progress.
>
> Reported-by: Sergio Lopez <slp@xxxxxxxxxx>
> Fixes: ac401cc78242 ("dax: New fault locking")
> Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
Looks good to me. Thanks for fixing this! Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/dax.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index f19d76a6a493..cc497519be83 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -676,7 +676,7 @@ static int __dax_invalidate_entry(struct address_space *mapping,
> mapping->nrexceptional--;
> ret = 1;
> out:
> - put_unlocked_entry(&xas, entry, WAKE_NEXT);
> + put_unlocked_entry(&xas, entry, WAKE_ALL);
> xas_unlock_irq(&xas);
> return ret;
> }
> --
> 2.25.4
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
