Re: [PATCH] btrfs: replace a wrong memset() with memzero_page()

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Mar 29, 2022 at 06:49:10PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/3/29 17:57, Filipe Manana wrote:
> > On Mon, Mar 28, 2022 at 08:51:21PM +0200, David Sterba wrote:
> > > On Fri, Mar 25, 2022 at 05:37:59PM +0800, Qu Wenruo wrote:
> > > > The original code is not really setting the memory to 0x00 but 0x01.
> > > > 
> > > > To prevent such problem from happening, use memzero_page() instead.
> > > 
> > > This should at least mention we think that setting it to 0 is right, as
> > > you call it wrong but give no hint why it's thought to be wrong.
> > 
> > My guess is that something different from zero makes it easier to spot
> > the problem in user space, as 0 is not uncommon (holes, prealloced extents)
> > and may get unnoticed by applications/users.
> 
> OK, that makes some sense.
> 
> But shouldn't user space tool get an -EIO directly?

It should.

But even if applications get -EIO, they may often ignore return values.
It's their fault, but if we can make it less likely that errors are not noticed,
the better. I think we all did often, ignore all or just some return values
from read(), write(), open(), etc.

One recent example is the MariaDB case with io-uring. They were reporting
corruption to the users, but the problem is that didn't properly check
return values, ignoring partial reads and treating them as success:

https://jira.mariadb.org/browse/MDEV-27900?focusedCommentId=216582&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-216582

The data was fine, not corrupted, they just didn't deal with partial reads
and then read the remaining data when a read returns less data than expected.



> 
> As the corrupted range won't have PageUptodate set anyway.
> 
> Thanks,
> Qu
> > 
> > I don't see a good reason to change this behaviour. Maybe it's just the
> > label name 'zeroit' that makes it confusing. >
> > > 
> > > > Since we're here, also make @len const since it's just sectorsize.
> > > 
> > > Please don't do that, adding const is fine when the line gets touched
> > > but otherwise adding it to an unrelated fix is not what I want to
> > > encourage.
> >