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;
Date:   Thu, 22 Apr 2021 13:46:34 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        William Kucharski <william.kucharski@oracle.com>,
        Christoph Hellwig <hch@lst.de>, Jan Kara <jack@suse.cz>,
        Dave Chinner <dchinner@redhat.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH 2/2] mm/filemap: fix mapping_seek_hole_data on THP &
 32-bit
In-Reply-To: <alpine.LSU.2.11.2104212253000.4412@eggly.anvils>
Message-ID: <alpine.LSU.2.11.2104221338410.1170@eggly.anvils>
References: <alpine.LSU.2.11.2104211723580.3299@eggly.anvils> <alpine.LSU.2.11.2104211737410.3299@eggly.anvils> <20210422011631.GL3596236@casper.infradead.org> <alpine.LSU.2.11.2104212253000.4412@eggly.anvils>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Precedence: bulk

On Wed, 21 Apr 2021, Hugh Dickins wrote:
> On Thu, 22 Apr 2021, Matthew Wilcox wrote:
> > On Wed, Apr 21, 2021 at 05:39:14PM -0700, Hugh Dickins wrote:
> > > No problem on 64-bit without huge pages, but xfstests generic/285
> > > and other SEEK_HOLE/SEEK_DATA tests have regressed on huge tmpfs,
> > > and on 32-bit architectures, with the new mapping_seek_hole_data().
> > > Several different bugs turned out to need fixing.
> > > 
> > > u64 casts added to stop unfortunate sign-extension when shifting
> > > (and let's use shifts throughout, rather than mixed with * and /).
> > 
> > That confuses me.  loff_t is a signed long long, but it can't be negative
> > (... right?)  So how does casting it to an u64 before dividing by
> > PAGE_SIZE help?
> 
> That is a good question. Sprinkling u64s was the first thing I tried,
> and I'd swear that it made a good difference at the time; but perhaps
> that was all down to just the one on xas.xa_index << PAGE_SHIFT. Or
> is it possible that one of the other bugs led to a negative loff_t,
> and the casts got better behaviour out of that? Doubtful.
> 
> What I certainly recall from yesterday was leaving out one (which?)
> of the casts as unnecessary, and wasting quite a bit of time until I
> put it back in. Did I really choose precisely the only one necessary?
> 
> Taking most of them out did give me good quick runs just now: I'll
> go over them again and try full runs on all machines. You'll think me
> crazy, but yesterday's experience leaves me reluctant to change without
> full testing - but agree it's not good to leave ignorant magic in.

And you'll be unsurprised to hear that the test runs went fine,
with all but one of those u64 casts removed. And I did locate the
version of filemap.c where I'd left out one "unnecessary" cast:
I had indeed chosen to remove the only one that's necessary.

v2 coming up now, thanks,

Hugh