linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	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: Wed, 28 Oct 2020 21:29:22 +0100	[thread overview]
Message-ID: <CAJfpegsaLrbJ7bjJVBC3=vLzWZcF+GtTpGjVKYYOE3mjKyuVAw@mail.gmail.com> (raw)
In-Reply-To: <20201021201249.GB442437@redhat.com>

On Wed, Oct 21, 2020 at 10:12 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>

> D. If one does a partial page write of a page which is not uptodate, then
>    keep page locked and do not try to send multiple pages in that write.
>    If page is uptodate, then release page lock and continue to add more
>    pages to same request.
>
> IOW, if head page is partial (and it is not uptodate), we will just
> send first WRITE with head page. Rest of the pages will go in second
> WRITE and tail page could be locked if it was a partial write and
> page was not uptodate. Please have a look at attached patch.

Looks good.

> I still some concerns though with error handling. Not sure what to
> do about it.
>
> 1. What happens if WRITE fails. If we are writing a full page and we
>   already marked it as Uptodate (And not dirty), then we have page
>   cache in page where we wrote data but could not send it to disk
>   (and did not mark dirty as well). So if a user reads the page
>   back it might get cache copy or get old copy from disk (if page
>   cache copy was released).

AFAICS this is what happens on write failure in current code IF the
page was uptodate previously.   Moving the SetPageUptodate() before
the WRITE makes this happen in all cases.

On write failure the page the uptodate flag should be cleared, which
should partially solve the above issue.  There still remains a window
where a concurrent read or load would get the wrong data, but I don't
think anybody cares (same happens with buffered write).

> 2. What happens if it is a partial page write to an Uptodate page
>   in cache and that WRITE fails. Now we have same error scenario
>   as 1. In fact this is true for even current code and not
>   necessarily a new scenario.

Same as above: need to clear uptodate flag.

> 3. Current code marks a page Uptodate upon WRITE completion if
>    it was full page WRITE. What if page was uptodate to begin
>    with and write fails. So current code will not mark it
>    Uptodate but it is already uptodate and we have same problem as 1.

Yes.

> Apart from above, there are some other concerns as well.
>
> So with this patch, if a page is Uptodate we drop lock and send WRITE.
> Otherwise we keep page lock and send WRITE. This should probably be
> fine from read or fault read point of view. Given we are holding inode
> lock, that means write path is not a problem as well. But
>
> What if page is redirtied through a write mapping
> -------------------------------------------------
> If page is redirtied through writable mmap, then two writes for same
> page can go in any order. But in synchronous write we are carrying
> pointer to page cache page, so it probably does not matter. We will
> just write same data twice.

It's not that simple.  Data dirtied through an mmap will be written
back using a temporary page.  So a WRITE request with such a page can
be in flight while a write(2) triggers a synchronous WRITE request for
the same data.  The two WRITEs are not ordered in any way and in fact
the page lock doesn't help, so this is not a new issue.   OTOH this
does not appear to be a problem in real life, since without msync() it
is not guaranteed that the memory mapping is synchronized with the
backing file (Linux has stronger guarantees, but test suites such as
fsx assume the lesser guarantees by POSIX).

This could be fixed to conform to the stronger coherency guarantee by
calling fuse_wait_on_page_writeback() after having gotten a locked
page in the synchronous writeback path.

>
> What about races with direct_IO read
> ------------------------------------
> If a WRITE is in progress, it is probably not marked dirty so
> generic_file_read_iter() will probably not block on
> filemap_write_and_wait_range() and continue mapping->a_ops->direct_IO().
> And that means it can read previous disk data before this WRITE is
> complete.

Synchronous write vs. direct IO read shouldn't be a problem as long as
the server provides a coherent view of the file to both.

Thanks,
Miklos


  reply	other threads:[~2020-10-28 20:29 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
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 [this message]
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='CAJfpegsaLrbJ7bjJVBC3=vLzWZcF+GtTpGjVKYYOE3mjKyuVAw@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).