Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <807bb470f90bae5dcd80a29020d38f6b5dd6ef8e.1616826872.git.baolin.wang@linux.alibaba.com>
 <f72f28cd-06b5-fb84-c7ce-ad1a3d14c016@linux.alibaba.com> <CAJfpegtJ6100CS34+MSi8Rn_NMRGHw5vxbs+fOHBBj8GZLEexw@mail.gmail.com>
 <CA+a=Yy4Ea6Vn7md2KxGc_Tkxx04Ck-JCBL7qz-JWecJ9W2nT_g@mail.gmail.com> <CAJfpegtXJ=waad2SNtru90Nn6f4yOkRD5Pot9K-13z249PjFgg@mail.gmail.com>
In-Reply-To: <CAJfpegtXJ=waad2SNtru90Nn6f4yOkRD5Pot9K-13z249PjFgg@mail.gmail.com>
From:   Peng Tao <bergwolf@gmail.com>
Date:   Thu, 15 Apr 2021 20:30:40 +0800
Message-ID: <CA+a=Yy57TdKEpEn0SC8zBCm8KmMuAYwSDqS=vtownzyt9qD6bA@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] fuse: Fix possible deadlock when writing back
 dirty pages
To:     Miklos Szeredi <miklos@szeredi.hu>
Cc:     Baolin Wang <baolin.wang@linux.alibaba.com>,
        Peng Tao <tao.peng@linux.alibaba.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Apr 14, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Apr 14, 2021 at 2:22 PM Peng Tao <bergwolf@gmail.com> wrote:
> >
>
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -1117,17 +1117,12 @@ static ssize_t fuse_send_write_pages(str
> > >       count = ia->write.out.size;
> > >       for (i = 0; i < ap->num_pages; i++) {
> > >               struct page *page = ap->pages[i];
> > > +             bool page_locked = ap->page_locked && (i == ap->num_pages - 1);
> > Any reason for just handling the last locked page in the page array?
> > To be specific, it look like the first page in the array can also be
> > partial dirty and locked?
>
> In that case the first partial page will be locked, and it'll break
> out of the loop...
>
> > >
> > > -             if (!err && !offset && count >= PAGE_SIZE)
> > > -                     SetPageUptodate(page);
> > > -
> > > -             if (count > PAGE_SIZE - offset)
> > > -                     count -= PAGE_SIZE - offset;
> > > -             else
> > > -                     count = 0;
> > > -             offset = 0;
> > > -
> > > -             unlock_page(page);
> > > +             if (err)
> > > +                     ClearPageUptodate(page);
> > > +             if (page_locked)
> > > +                     unlock_page(page);
> > >               put_page(page);
> > >       }
> > >
> > > @@ -1191,6 +1186,16 @@ static ssize_t fuse_fill_write_pages(str
> > >               if (offset == PAGE_SIZE)
> > >                       offset = 0;
> > >
> > > +             /* If we copied full page, mark it uptodate */
> > > +             if (tmp == PAGE_SIZE)
> > > +                     SetPageUptodate(page);
> > > +
> > > +             if (PageUptodate(page)) {
> > > +                     unlock_page(page);
> > > +             } else {
> > > +                     ap->page_locked = true;
> > > +                     break;
>
> ... here, and send it as a separate WRITE request.
>
> So the multi-page case with a partial & non-uptodate head page will
> always result in the write request being split into two (even if
> there's no partial tail page).

Ah, good point! Thanks for the explanation. I agree that it can fix
the deadlock issue here.

One thing I'm still uncertain about is that fuse used to fill the
page, wait for page writeback, and send it to userspace all with the
page locked, which is kind of like a stable page mechanism for FUSE.
With the above change, we no longer lock a PG_uptodate page when
waiting for its writeback and sending it to userspace. Then the page
can be modified when being sent to userspace. Is it acceptable?

Cheers,
Tao
-- 
Into Sth. Rich & Strange