Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <CGME20230915095133eucas1p267bade2888b7fcd2e1ea8e13e21c495f@eucas1p2.samsung.com>
 <20230915095042.1320180-1-da.gomez@samsung.com> <20230915095042.1320180-7-da.gomez@samsung.com>
 <CAJD7tkbU20tyGxtdL-cqJxrjf38ObG_dUttZdLstH3O2sUTKzw@mail.gmail.com>
 <20230918075758.vlufrhq22es2dhuu@sarkhan> <CAJD7tkZSST8Kc6duUWt6a9igrsn=ucUPSVPWWGDWEUxBs3b4bg@mail.gmail.com>
 <20230919132633.v2mvuaxp2w76zoed@sarkhan> <CAJD7tkaELyZXsUP+c=DKg9k-FeFTTRS+_9diK5fyTNdfDAykmQ@mail.gmail.com>
 <ZQoW0MVh/esJkU6H@bombadil.infradead.org>
In-Reply-To: <ZQoW0MVh/esJkU6H@bombadil.infradead.org>
From:   Yosry Ahmed <yosryahmed@google.com>
Date:   Tue, 19 Sep 2023 14:51:40 -0700
Message-ID: <CAJD7tkZjpYh=n4UrYmpMUVz2OvBX9PVzK+aP1gKsNoycnkzRag@mail.gmail.com>
Subject: Re: [PATCH 6/6] shmem: add large folios support to the write path
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Daniel Gomez <da.gomez@samsung.com>,
        "minchan@kernel.org" <minchan@kernel.org>,
        "senozhatsky@chromium.org" <senozhatsky@chromium.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "djwong@kernel.org" <djwong@kernel.org>,
        "willy@infradead.org" <willy@infradead.org>,
        "hughd@google.com" <hughd@google.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "gost.dev@samsung.com" <gost.dev@samsung.com>,
        Pankaj Raghav <p.raghav@samsung.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Sep 19, 2023 at 2:47=E2=80=AFPM Luis Chamberlain <mcgrof@kernel.org=
> wrote:
>
> On Tue, Sep 19, 2023 at 09:00:16AM -0700, Yosry Ahmed wrote:
> > On Tue, Sep 19, 2023 at 6:27=E2=80=AFAM Daniel Gomez <da.gomez@samsung.=
com> wrote:
> > >
> > > On Mon, Sep 18, 2023 at 11:55:34AM -0700, Yosry Ahmed wrote:
> > > > On Mon, Sep 18, 2023 at 1:00=E2=80=AFAM Daniel Gomez <da.gomez@sams=
ung.com> wrote:
> > > > >
> > > > > On Fri, Sep 15, 2023 at 11:26:37AM -0700, Yosry Ahmed wrote:
> > > > > > On Fri, Sep 15, 2023 at 2:51=E2=80=AFAM Daniel Gomez <da.gomez@=
samsung.com> wrote:
> > > > > > >
> > > > > > > Add large folio support for shmem write path matching the sam=
e high
> > > > > > > order preference mechanism used for iomap buffered IO path as=
 used in
> > > > > > > __filemap_get_folio().
> > > > > > >
> > > > > > > Use the __folio_get_max_order to get a hint for the order of =
the folio
> > > > > > > based on file size which takes care of the mapping requiremen=
ts.
> > > > > > >
> > > > > > > Swap does not support high order folios for now, so make it o=
rder 0 in
> > > > > > > case swap is enabled.
> > > > > >
> > > > > > I didn't take a close look at the series, but I am not sure I
> > > > > > understand the rationale here. Reclaim will split high order sh=
mem
> > > > > > folios anyway, right?
> > > > >
> > > > > For context, this is part of the enablement of large block sizes =
(LBS)
> > > > > effort [1][2][3], so the assumption here is that the kernel will
> > > > > reclaim memory with the same (large) block sizes that were writte=
n to
> > > > > the device.
> > > > >
> > > > > I'll add more context in the V2.
> > > > >
> > > > > [1] https://protect2.fireeye.com/v1/url?k=3Da80aab33-c981be05-a80=
b207c-000babff9b5d-b656d8860b04562f&q=3D1&e=3D46666acf-d70d-4e8d-8d00-b0278=
08ae400&u=3Dhttps%3A%2F%2Fkernelnewbies.org%2FKernelProjects%2Flarge-block-=
size
> > > > > [2] https://protect2.fireeye.com/v1/url?k=3D3f753ca2-5efe2994-3f7=
4b7ed-000babff9b5d-e678f885471555e3&q=3D1&e=3D46666acf-d70d-4e8d-8d00-b0278=
08ae400&u=3Dhttps%3A%2F%2Fdocs.google.com%2Fspreadsheets%2Fd%2Fe%2F2PACX-1v=
S7sQfw90S00l2rfOKm83Jlg0px8KxMQE4HHp_DKRGbAGcAV-xu6LITHBEc4xzVh9wLH6WM2lR0c=
ZS8%2Fpubhtml%23
> > > > > [3] https://lore.kernel.org/all/ZQfbHloBUpDh+zCg@dread.disaster.a=
rea/
> > > > > >
> > > > > > It seems like we only enable high order folios if the "noswap" =
mount
> > > > > > option is used, which is fairly recent. I doubt it is widely us=
ed.
> > > > >
> > > > > For now, I skipped the swap path as it currently lacks support fo=
r
> > > > > high order folios. But I'm currently looking into it as part of t=
he LBS
> > > > > effort (please check spreadsheet at [2] for that).
> > > >
> > > > Thanks for the context, but I am not sure I understand.
> > > >
> > > > IIUC we are skipping allocating large folios in shmem if swap is
> > > > enabled in this patch. Swap does not support swapping out large fol=
ios
> > > > as a whole (except THPs), but page reclaim will split those large
> > > > folios and swap them out as order-0 pages anyway. So I am not sure =
I
> > > > understand why we need to skip allocating large folios if swap is
> > > > enabled.
> > >
> > > I lifted noswap condition and retested it again on top of 230918 and
> > > there is some regression. So, based on the results I guess the initia=
l
> > > requirement may be the way to go. But what do you think?
> > >
> > > Here the logs:
> > > * shmem-large-folios-swap: https://gitlab.com/-/snippets/3600360
> > > * shmem-baseline-swap : https://gitlab.com/-/snippets/3600362
> > >
> > > -Failures: generic/080 generic/126 generic/193 generic/633 generic/68=
9
> > > -Failed 5 of 730 tests
> > > \ No newline at end of file
> > > +Failures: generic/080 generic/103 generic/126 generic/193 generic/28=
5 generic/436 generic/619 generic/633 generic/689
> > > +Failed 9 of 730 tests
> > > \ No newline at end of file
> > > >
> >
> > I am not really familiar with these tests so I cannot really tell
> > what's going on. I can see "swapfiles are not supported" in the logs
> > though, so it seems like we are seeing extra failures by just lifting
> > "noswap" even without actually swapping. I am curious if this is just
> > hiding a different issue, I would at least try to understand what's
> > happening.
> >
> > Anyway, I don't have enough context here to be useful. I was just
> > making an observation about reclaim splitting shmem folios to swap
> > them out as order-0 pages, and asking why this is needed based on
> > that. I will leave it up to you and the reviewers to decide if there's
> > anything interesting here.
>
> The tests which are failing seem be related to permissions, I could not
> immediate decipher why, because as you suggest we'd just be doing the
> silly thing of splitting large folios on writepage.
>
> I'd prefer we don't require swap until those regressions would be fixed.
>
> Note that part of the rationale to enable this work is to eventually
> also extend swap code to support large order folios, so it is not like
> this would be left as-is. It is just that it may take time to resolve
> the kinks with swap.
>
> So I'd stick to nowap for now.
>
> The above tests also don't stress swap too, and if we do that I would
> imagine we might see some other undesirable failures.
>
>  Luis

I thought we already have some notion of exercising swap with large
shmem folios from THPs, so this shouldn't be new, but perhaps I am
missing something.