Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[Qu Wenruo]

On 2022/6/24 20:49, David Sterba wrote:
> On Fri, Jun 24, 2022 at 02:23:34PM +0200, Christoph Hellwig wrote:
>> Since the page_mkwrite address space operation was added, starting with
>> commit 9637a5efd4fb ("[PATCH] add page_mkwrite() vm_operations method")
>> in 2006, the kernel does not just dirty random pages without telling
>> the file system.
>
> It does and there's a history behind the fixup worker. tl;dr it can't be
> removed, though every now and then somebody comes and tries to.
>
> On s390 the page status is tracked in two places, hw and in memory and
> this needs to be synchronized manually.
>
> On x86_64 it's not a simple reason but it happens as well in some edge
> case where the mappings get removed and dirty page is set deep in the
> arch mm code.  We've been chasing it long time ago, I don't recall exact
> details and it's been a painful experience.
>
> If there's been any change on the s390 side or in arch/x86/mm code I
> don't know but to be on the safe side, I strongly assume the fixup code
> is needed unless proven otherwise.

I'd say, if this can be a problem to btrfs, then all fs supporting COW
should also be affected, and should have similar workaround.


Furthermore, this means we can get a page dirtied without us knowing.

This is a super big surprise to any fs, and should be properly
documented, not just leaving some seemly dead and special code in some
random fs.

Furthermore, I'm not sure even if handling this in a fs level is correct.
This looks like more a MM problem to me then.


I totally understand it's a pain to debug such lowlevel bug, but
shouldn't we have a proper regression for it then?

Instead of just keeping what we know works, I really want to handle this
old case/bug in a more modern way.

Thanks,
Qu

Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[David Sterba]

On Fri, Jun 24, 2022 at 09:12:44PM +0800, Qu Wenruo wrote:
> On 2022/6/24 20:49, David Sterba wrote:
> > On Fri, Jun 24, 2022 at 02:23:34PM +0200, Christoph Hellwig wrote:
> >> Since the page_mkwrite address space operation was added, starting with
> >> commit 9637a5efd4fb ("[PATCH] add page_mkwrite() vm_operations method")
> >> in 2006, the kernel does not just dirty random pages without telling
> >> the file system.
> >
> > It does and there's a history behind the fixup worker. tl;dr it can't be
> > removed, though every now and then somebody comes and tries to.
> >
> > On s390 the page status is tracked in two places, hw and in memory and
> > this needs to be synchronized manually.
> >
> > On x86_64 it's not a simple reason but it happens as well in some edge
> > case where the mappings get removed and dirty page is set deep in the
> > arch mm code.  We've been chasing it long time ago, I don't recall exact
> > details and it's been a painful experience.
> >
> > If there's been any change on the s390 side or in arch/x86/mm code I
> > don't know but to be on the safe side, I strongly assume the fixup code
> > is needed unless proven otherwise.
> 
> I'd say, if this can be a problem to btrfs, then all fs supporting COW
> should also be affected, and should have similar workaround.

Probably yes.

> Furthermore, this means we can get a page dirtied without us knowing.

This should not happen because we do have the detection of the page and
extent state mismatch and the fixup worker makes things right again.

> This is a super big surprise to any fs, and should be properly
> documented, not just leaving some seemly dead and special code in some
> random fs.

You seem to be a non-believer that the bug is real and calling the code
dead. Each filesystem should validate the implementation agains the
platform where it is and btrfs once found the hard way that there are
some corner cases where structures get out of sync.

> Furthermore, I'm not sure even if handling this in a fs level is correct.
> This looks like more a MM problem to me then.
> 
> 
> I totally understand it's a pain to debug such lowlevel bug, but
> shouldn't we have a proper regression for it then?

The regression test is generic/208 and it was not reliable at all, it
fired randomly once a week or month, there used to be a BUG() in the
fixup worker callback.

> Instead of just keeping what we know works, I really want to handle this
> old case/bug in a more modern way.

As long as the guarantees stay the same, then fine. We need to be able
to detect the unexpected dirty bit and have a way to react to it.

f4b1363cae43 ("btrfs: do not do delalloc reservation under page lock")
25f3c5021985 ("Btrfs: keep pages dirty when using btrfs_writepage_fixup_worker")
1d53c9e67230 ("Btrfs: only associate the locked page with one async_chunk struct")

And the commit that fixed it:

87826df0ec36 ("btrfs: delalloc for page dirtied out-of-band in fixup worker")

You can find several reports in the mailing list archives (search term
btrfs_writepage_fixup_worker):

https://lore.kernel.org/linux-btrfs/1295053074.15265.6.camel@mercury.localdomain

https://lore.kernel.org/linux-btrfs/20110701174436.GA8352@yahoo.fr

https://lore.kernel.org/linux-btrfs/j0k65i$29a$1@dough.gmane.org

https://lore.kernel.org/linux-btrfs/CAO47_--H0+6bu4qQ2QA9gZcHvGVWO4QUGCAb3+9a5Kg3+23UiQ@mail.gmail.com

https://lore.kernel.org/linux-btrfs/vqfmv8-9ch.ln1@hurikhan.ath.cx

Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[Qu Wenruo]

On 2022/6/24 21:27, David Sterba wrote:
> On Fri, Jun 24, 2022 at 09:12:44PM +0800, Qu Wenruo wrote:
>> On 2022/6/24 20:49, David Sterba wrote:
>>> On Fri, Jun 24, 2022 at 02:23:34PM +0200, Christoph Hellwig wrote:
>>>> Since the page_mkwrite address space operation was added, starting with
>>>> commit 9637a5efd4fb ("[PATCH] add page_mkwrite() vm_operations method")
>>>> in 2006, the kernel does not just dirty random pages without telling
>>>> the file system.
>>>
>>> It does and there's a history behind the fixup worker. tl;dr it can't be
>>> removed, though every now and then somebody comes and tries to.
>>>
>>> On s390 the page status is tracked in two places, hw and in memory and
>>> this needs to be synchronized manually.
>>>
>>> On x86_64 it's not a simple reason but it happens as well in some edge
>>> case where the mappings get removed and dirty page is set deep in the
>>> arch mm code.  We've been chasing it long time ago, I don't recall exact
>>> details and it's been a painful experience.
>>>
>>> If there's been any change on the s390 side or in arch/x86/mm code I
>>> don't know but to be on the safe side, I strongly assume the fixup code
>>> is needed unless proven otherwise.
>>
>> I'd say, if this can be a problem to btrfs, then all fs supporting COW
>> should also be affected, and should have similar workaround.
>
> Probably yes.
>
>> Furthermore, this means we can get a page dirtied without us knowing.
>
> This should not happen because we do have the detection of the page and
> extent state mismatch and the fixup worker makes things right again.
>
>> This is a super big surprise to any fs, and should be properly
>> documented, not just leaving some seemly dead and special code in some
>> random fs.
>
> You seem to be a non-believer that the bug is real and calling the code
> dead.

Nope, as Jan mentioned RDMA, it immediately light the bulb in my head.

Now I'm totally convinced we can have page marked dirty without proper
notification to fs.

So now I think this is real bug.

But unfortunately the code itself has no concrete reasons on which cases
this can happen, just mentioned kernel can mark page dirty (seemly
randomly).

Thus it "looks like" a dead code.


> Each filesystem should validate the implementation agains the
> platform where it is and btrfs once found the hard way that there are
> some corner cases where structures get out of sync.

In fact, from the fs point of view, there are quite some expectation on
its interfaces, if there is a surprise and such problem is no long
really specific to btrfs, then it should be addressed more generically.

>
>> Furthermore, I'm not sure even if handling this in a fs level is correct.
>> This looks like more a MM problem to me then.
>>
>>
>> I totally understand it's a pain to debug such lowlevel bug, but
>> shouldn't we have a proper regression for it then?
>
> The regression test is generic/208 and it was not reliable at all, it
> fired randomly once a week or month, there used to be a BUG() in the
> fixup worker callback.

And it doesn't have any comment even related to this unexpected dirty pages.

>
>> Instead of just keeping what we know works, I really want to handle this
>> old case/bug in a more modern way.
>
> As long as the guarantees stay the same, then fine. We need to be able
> to detect the unexpected dirty bit and have a way to react to it.
>
> f4b1363cae43 ("btrfs: do not do delalloc reservation under page lock")
> 25f3c5021985 ("Btrfs: keep pages dirty when using btrfs_writepage_fixup_worker")
> 1d53c9e67230 ("Btrfs: only associate the locked page with one async_chunk struct")
>
> And the commit that fixed it:
>
> 87826df0ec36 ("btrfs: delalloc for page dirtied out-of-band in fixup worker")
>
> You can find several reports in the mailing list archives (search term
> btrfs_writepage_fixup_worker):

To me, a proper and modern solution is not to rely on super old reports
(although they are definitely helpful as a record), but proper explanation.

Thanks to Jan, RDMA would be a very direct example for this.

Although personally speaking, I still think we should limit on who can
set a page from page cache dirty.
(AKA, ensuring fs receives notification on every dirtied page)

Thanks,
Qu

>
> https://lore.kernel.org/linux-btrfs/1295053074.15265.6.camel@mercury.localdomain
>
> https://lore.kernel.org/linux-btrfs/20110701174436.GA8352@yahoo.fr
>
> https://lore.kernel.org/linux-btrfs/j0k65i$29a$1@dough.gmane.org
>
> https://lore.kernel.org/linux-btrfs/CAO47_--H0+6bu4qQ2QA9gZcHvGVWO4QUGCAb3+9a5Kg3+23UiQ@mail.gmail.com
>
> https://lore.kernel.org/linux-btrfs/vqfmv8-9ch.ln1@hurikhan.ath.cx

Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[Christoph Hellwig]

On Tue, Jun 28, 2022 at 01:53:56PM +0200, David Sterba wrote:
> This would work only for the higher level API where eg. RDMA notifies
> the filesystem, but there's still the s390 case that is part of the
> hardware architecture. The fixup worker is there as a safety for all
> other cases, I'm not fine removing or ignoring it.

I'd really like to have a confirmation of this whole s390 theory.
s390 does treat some dirtying different than the other architectures,
but none of that should leak into the file system API if any way that
bypasses ->page_mkwrite.

Because if it did most file systems would be completely broken on
s390.

Re: [PATCH] btrfs: remove btrfs_writepage_cow_fixup

[Gerald Schaefer]

On Wed, 29 Jun 2022 09:58:37 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Tue, Jun 28, 2022 at 01:53:56PM +0200, David Sterba wrote:
> > This would work only for the higher level API where eg. RDMA notifies
> > the filesystem, but there's still the s390 case that is part of the
> > hardware architecture. The fixup worker is there as a safety for all
> > other cases, I'm not fine removing or ignoring it.  
> 
> I'd really like to have a confirmation of this whole s390 theory.
> s390 does treat some dirtying different than the other architectures,
> but none of that should leak into the file system API if any way that
> bypasses ->page_mkwrite.
> 
> Because if it did most file systems would be completely broken on
> s390.

Could you please be more specific about what exactly you mean with
"the s390 case that is part of the hardware architecture"?

One thing that s390 might handle different from others, is that it
is not using a HW dirty bit in the PTE, but instead a fault-triggered
SW dirty bit.

E.g. pte_mkwrite() will mark a PTE as writable (via another SW bit),
but not clear the HW protection bit, which would then generate a
fault on first write access. In handle_pte_fault(), the PTE would
then be marked as dirty via pte_mkdirty(), which also clears the HW
protection bit, at least for pte_write() PTEs.
For the !pte_write() COW case, we would go through do_wp_page() like
everybody else, but probably still end up in some pte_mkdirty()
eventually, to avoid getting another fault.

Not being familiar with either btrfs, any other fs, or RDMA, I cannot
really follow the discussion here. Still it seems to me that you are
not talking about special s390 HW architecture regarding PTE, but
rather about some (struct) page dirtying on the COW path, which should
be completely common code and not subject to any s390 special case.

Somewhere in this thread it was also mentioned that "s390 can not do
page flags update atomically", which I can not confirm, in case this
was the question. The code in include/linux/page-flags.h seems to
use normal (arch)_test/set/clear_bit operations, which should always be
atomic on s390.