Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 4 Dec 2022 16:45:35 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Rui Wang <r@hev.cc>
cc:     Andrew Morton <akpm@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>,
        Guoqi Chen <chenguoqic@163.com>,
        Huacai Chen <chenhuacai@loongson.cn>, Rui Wang <kernel@hev.cc>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH] mm/shmem: Fix undo range for failed fallocate
In-Reply-To: <9984f58e-826-74c6-1cd4-65366cc01549@google.com>
Message-ID: <5889e4e3-e054-7654-1436-8d2bcbefe3c6@google.com>
References: <20221101032248.819360-1-kernel@hev.cc> <Y2KBovUHODJJ8ZnV@casper.infradead.org> <CAHirt9h2CrLhYML3XW=Vj4=BD5eVDoRAbULVGgNbEdYnAzwCzA@mail.gmail.com> <9984f58e-826-74c6-1cd4-65366cc01549@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 22 Nov 2022, Hugh Dickins wrote:
> On Thu, 3 Nov 2022, hev wrote:
> > On Wed, Nov 2, 2022 at 10:41 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > On Tue, Nov 01, 2022 at 11:22:48AM +0800, Rui Wang wrote:
> > > > This patch fixes data loss caused by the fallocate system
> > > > call interrupted by a signal.
> > > >
> > > > Bug: https://lore.kernel.org/linux-mm/33b85d82.7764.1842e9ab207.Coremail.chenguoqic@163.com/
> > > > Fixes: b9a8a4195c7d ("truncate,shmem: Handle truncates that split large folios")
> > >
> > > How does that commit introduce this bug?
> > 
> > In the test case[1], we created a file that contains non-zero data
> > from offset 0 to A-1. and a process try to expand this file by
> > fallocate(fd, 0, 0, B), B > A.
> > Concurrently, another process try to interrupt this fallocate syscall
> > by a signal. I think the expected results are:
> > 
> > 1. The file is not expanded and file size is A, and the data from
> > offset 0 to A-1 is not changed.
> > 2. The file is expanded and the data from offset 0 to A-1 is not
> > changed, and from A to B-1 contains zeros.
> > 
> > Now, the unexpected result is that the file is not expanded and the
> > data that from offset 0 to A-1 is changed by
> > truncate_inode_partial_folio that called
> > from shmem_undo_range with unfalloc = true.
> > 
> > This issue is only reproduced when file on tmpfs, and begin from this
> > commit: b9a8a4195c7d ("truncate,shmem: Handle truncates that split
> > large folios")
> 
> Like Matthew, I was sceptical at first.
> 
> But I currently think that you have discovered something important, and
> that your patch is the correct fix to it; but I'm still rather confused,
> and want to do some more thinking and testing: this mail is mainly to
> signal to Matthew that I'm on it, so he doesn't have to rush to look
> at it when he's back.
> 
> I was able to reproduce it with the test case, once I multiplied both
> of the usleep intervals by 10 - before that, it was too difficult for
> it to complete a fallocate: guess the timing is different on my x86 box.

Thanks, I needed that breathing space to get back to thinking it through.

My main concern was, how did Matthew and I come to miss this unfalloc
issue when the folio commit went in?  Speaking for myself (but quite
likely for Matthew too), it's because there was never a need for
special unfalloc treatment in the partial page handling there before,
so we didn't even think of adding any when that got updated.

But when the partial page handling got turned into partial folio
handling, it came to do a lot more than before: it's tricky stuff,
and Matthew intentionally moved all the difficulty there into the
partial folio block; whereas before it had been just as tricky,
but hidden away in the shmem_punch_compound() call from inside the
loop which follows.  And was subject there to the unfalloc exception.

So that sort of satisfied me, how we came to overlook it.

Your patch worked for me, and until yesterday I was intending to
send it in as is.  But then realized that, although it's a peculiar
exception to the standard processing there, unfalloc actually has a
much simpler job to do: just remove any !uptodate folios from the range.
It has no need whatsoever for the zeroing and splitting involved in
truncate_inode_partial_folio(), and I really wanted to avoid having
to think through all that again - easier just to skip it all.

So the patch I'm about to send to Andrew is not quite yours:
but many thanks to you and to Guoqi Chen for revealing this issue.

Hugh