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:   Mon, 16 May 2022 13:55:25 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Hsin-Yi Wang <hsinyi@chromium.org>
Cc:     Phillip Lougher <phillip@squashfs.org.uk>,
        Xiongwei Song <Xiongwei.Song@windriver.com>,
        Zheng Liang <zhengliang6@huawei.com>,
        Zhang Yi <yi.zhang@huawei.com>, Hou Tao <houtao1@huawei.com>,
        Miao Xie <miaoxie@huawei.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "linux-mm @ kvack . org" <linux-mm@kvack.org>,
        "squashfs-devel @ lists . sourceforge . net" 
        <squashfs-devel@lists.sourceforge.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] squashfs: implement readahead
Message-ID: <YoJJvabBCTJHryGm@casper.infradead.org>
References: <20220516105100.1412740-1-hsinyi@chromium.org>
 <20220516105100.1412740-3-hsinyi@chromium.org>
 <CAJMQK-jjNRuZFOJrd2knQHwPERaJ64pyys+14cCVyfdsP1GZSQ@mail.gmail.com>
 <YoJFOpbMYSEOGfXy@casper.infradead.org>
 <CAJMQK-iDFJGHzmJQ0VZRXL-O8MBeqLFS-b0M-6bm3ncZ4TgMJQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJMQK-iDFJGHzmJQ0VZRXL-O8MBeqLFS-b0M-6bm3ncZ4TgMJQ@mail.gmail.com>
Precedence: bulk

On Mon, May 16, 2022 at 08:47:52PM +0800, Hsin-Yi Wang wrote:
> On Mon, May 16, 2022 at 8:36 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, May 16, 2022 at 07:04:08PM +0800, Hsin-Yi Wang wrote:
> > > > +       loff_t req_end = readahead_pos(ractl) + readahead_length(ractl);
> > > > +       loff_t start = readahead_pos(ractl) &~ mask;
> > > > +       size_t len = readahead_length(ractl) + readahead_pos(ractl) - start;
> > > > +       struct squashfs_page_actor *actor;
> > > > +       unsigned int nr_pages = 0;
> > > > +       struct page **pages;
> > > > +       u64 block = 0;
> > > > +       int bsize, res, i, index;
> > > > +       int file_end = i_size_read(inode) >> msblk->block_log;
> > > > +       unsigned int max_pages = 1UL << shift;
> > > > +
> > > > +       readahead_expand(ractl, start, (len | mask) + 1);
> > > > +
> > > > +       if (readahead_pos(ractl) + readahead_length(ractl) < req_end ||
> > > > +           file_end == 0)
> > > > +               return;
> >
> > What's the first half of this condition supposed to be checking for?
> > It seems to be checking whether readahead_expand() shrunk the range
> > covered by the ractl, but readahead_expand() never does that, so I'm
> > confused why you're checking for it.
> 
> hi Matthew,
> 
> This is to check if readahead_expand() expands as much as it's requested.
> I didn't encounter the mismatch so far in my testing. If this check is
> not necessary, it can be removed.

Then I think req_end is miscalculated?  It should surely be:

	req_end = start + (len | mask) + 1;

But I'm not sure that we should be failing under such circumstances.
For example, we may have been asked to read 1.5MB, attempt to round up
to 2MB, and fail.  But we can still submit a readahead for the first 1MB,
before leaving the second 512kB for readpage to handle.

So maybe we should just remove this check entirely.