linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Vivek Goyal <vgoyal@redhat.com>, Qian Cai <cai@lca.pw>,
	Hugh Dickins <hughd@google.com>,
	 Matthew Wilcox <willy@infradead.org>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	 Linux-MM <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	 linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Amir Goldstein <amir73il@gmail.com>
Subject: Re: Possible deadlock in fuse write path (Was: Re: [PATCH 0/4] Some more lock_page work..)
Date: Fri, 16 Oct 2020 12:02:21 +0200	[thread overview]
Message-ID: <CAJfpegtAstEo+nYgT81swYZWdziaZP_40QGAXcTORqYwgeWNUA@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wj0vjx0jzaq5Gha-SmDKc3Hnog5LKX0eJZkudBvEQFAUA@mail.gmail.com>

On Thu, Oct 15, 2020 at 11:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Oct 15, 2020 at 12:55 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > I am wondering how should I fix this issue. Is it enough that I drop
> > the page lock (but keep the reference) inside the loop. And once copying
> > from user space is done, acquire page locks for all pages (Attached
> > a patch below).
>
> What is the page lock supposed to protect?
>
> Because whatever it protects, dropping the lock drops, and you'd need
> to re-check whatever the page lock was there for.
>
> > Or dropping page lock means that there are no guarantees that this
> > page did not get written back and removed from address space and
> > a new page has been placed at same offset. Does that mean I should
> > instead be looking up page cache again after copying from user
> > space is done.
>
> I don't know why fuse does multiple pages to begin with. Why can't it
> do whatever it does just one page at a time?
>
> But yes, you probably should look the page up again whenever you've
> unlocked it, because it might have been truncated or whatever.
>
> Not that this is purely about unlocking the page, not about "after
> copying from user space". The iov_iter_copy_from_user_atomic() part is
> safe - if that takes a page fault, it will just do a partial copy, it
> won't deadlock.
>
> So you can potentially do multiple pages, and keep them all locked,
> but only as long as the copies are all done with that
> "from_user_atomic()" case. Which normally works fine, since normal
> users will write stuff that they just generated, so it will all be
> there.
>
> It's only when that returns zero, and you do the fallback to pre-fault
> in any data with iov_iter_fault_in_readable() that you need to unlock
> _all_ pages (and once you do that, I don't see what possible advantage
> the multi-page array can have).
>
> Of course, the way that code is written, it always does the
> iov_iter_fault_in_readable() for each page - it's not written like
> some kind of "special case fallback thing".

This was added by commit ea9b9907b82a ("fuse: implement
perform_write") in v2.6.26 and remains essentially unchanged, AFAICS.
So this is an old bug indeed.

So what is the page lock protecting?   I think not truncation, because
inode_lock should be sufficient protection.

What it does after sending a synchronous WRITE and before unlocking
the pages is set the PG_uptodate flag, but only if the complete page
was really written, which is what the uptodate flag really says:  this
page is in sync with the underlying fs.

So I think the page lock here is trying to protect against concurrent
reads/faults on not uptodate pages.  I.e. until the WRITE request
completes it is unknown whether the page was really written or not, so
any reads must block until this state becomes known.  This logic falls
down on already cached pages, since they start out uptodate and the
write does not clear this flag.

So keeping the pages locked has dubious value: short writes don't seem
to work correctly anyway.  Which means that we can probably just set
the page uptodate right after filling it from the user buffer, and
unlock the page immediately.

Am I missing something?

Thanks,
Miklos


  reply	other threads:[~2020-10-16 10:02 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-13 19:59 [PATCH 0/4] Some more lock_page work Linus Torvalds
2020-10-13 20:03 ` Linus Torvalds
2020-10-14 13:05   ` Kirill A. Shutemov
2020-10-14 16:53     ` Linus Torvalds
2020-10-14 18:15       ` Matthew Wilcox
2020-10-15 10:41         ` Kirill A. Shutemov
2020-10-15  9:43       ` Kirill A. Shutemov
2020-10-15 16:44         ` Linus Torvalds
2020-10-14  5:50 ` Hugh Dickins
     [not found] ` <4794a3fa3742a5e84fb0f934944204b55730829b.camel@lca.pw>
2020-10-15  2:44   ` Linus Torvalds
2020-10-15 15:16     ` Possible deadlock in fuse write path (Was: Re: [PATCH 0/4] Some more lock_page work..) Vivek Goyal
2020-10-15 19:55       ` Vivek Goyal
2020-10-15 21:21         ` Linus Torvalds
2020-10-16 10:02           ` Miklos Szeredi [this message]
2020-10-16 12:27             ` Matthew Wilcox
2020-10-20 20:42             ` Vivek Goyal
2020-10-21  7:40               ` Miklos Szeredi
2020-10-21 20:12                 ` Vivek Goyal
2020-10-28 20:29                   ` Miklos Szeredi
2021-02-09 10:01                     ` Miklos Szeredi
2021-02-09 19:09                       ` Vivek Goyal
2020-10-16 18:19           ` Vivek Goyal
2020-10-16 18:24             ` Linus Torvalds
2020-10-16 18:24               ` Linus Torvalds
2020-10-16 23:03             ` 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=CAJfpegtAstEo+nYgT81swYZWdziaZP_40QGAXcTORqYwgeWNUA@mail.gmail.com \
    --to=miklos@szeredi.hu \
    --cc=akpm@linux-foundation.org \
    --cc=amir73il@gmail.com \
    --cc=cai@lca.pw \
    --cc=hughd@google.com \
    --cc=kirill@shutemov.name \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=torvalds@linux-foundation.org \
    --cc=vgoyal@redhat.com \
    --cc=willy@infradead.org \
    --subject='Re: Possible deadlock in fuse write path (Was: Re: [PATCH 0/4] Some more lock_page work..)' \
    /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

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).