Received-SPF: pass (google.com: domain of linux-kernel+bounces-158047-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240403021808.309900-1-vinicius.gomes@intel.com>
 <CAJfpeguqW4mPE9UyLmccisTex_gmwq6p9_6_EfVm-1oh6CrEBA@mail.gmail.com> <87frvay47x.fsf@intel.com>
In-Reply-To: <87frvay47x.fsf@intel.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 25 Apr 2024 09:06:08 +0300
Message-ID: <CAOQ4uxjdk47CVbihhsErzNQdUBiPshBOd8rg-PerBuOPjY=e5w@mail.gmail.com>
Subject: Re: [PATCH v1 0/3] overlayfs: Optimize override/revert creds
To: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>, brauner@kernel.org, hu1.chen@intel.com, 
	malini.bhandaru@intel.com, tim.c.chen@intel.com, mikko.ylinen@intel.com, 
	lizhen.you@intel.com, linux-unionfs@vger.kernel.org, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 24, 2024 at 10:01=E2=80=AFPM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> writes:
>
> > On Wed, 3 Apr 2024 at 04:18, Vinicius Costa Gomes
> > <vinicius.gomes@intel.com> wrote:
> >
> >>  - in ovl_rename() I had to manually call the "light" the overrides,
> >>    both using the guard() macro or using the non-light version causes
> >>    the workload to crash the kernel. I still have to investigate why
> >>    this is happening. Hints are appreciated.
> >
> > Don't know.  Well, there's nesting (in ovl_nlink_end()) but I don't
> > see why that should be an issue.
> >
> > I see why Amir suggested moving away from scoped guards, but that also
> > introduces the possibility of subtle bugs if we don't audit every one
> > of those sites carefully...
> >
> > Maybe patchset should be restructured to first do the
> > override_creds_light() conversion without guards, and then move over
> > to guards.   Or the other way round, I don't have a preference.  But
> > mixing these two independent changes doesn't sound like a great idea
> > in any case.
>
> Sounds good. Here's I am thinking:
>
> patch 1: introduce *_creds_light()
> patch 2: move backing-file.c to *_creds_light()
> patch 3: move overlayfs to *_creds_light()
> patch 4: introduce the guard helpers
> patch 5: move backing-file.c to the guard helpers
> patch 6: move overlayfs to the guard helpers
>
> (and yeah, the subject of the patches will be better than these ;-)
>
> Is this what you had in mind?
>

I think this series would make a lot of sense.
It first addresses the issue that motivated your work
and I expect patch 3 would be rather simple to review.

Please use your best judgement to break patch 6 into more chewable
pieces because the current ovl patch is quite large to review in one go.
I will leave it up to you to find the right balance.

Also w.r.t the guard() vs. scoped_guard() question, remember that
there is another option that may be better than either in some cases -
re-factoring to a helper with a guard().

One example that jumps to me is ovl_copyfile() - seems nicer
to add a guard() in all the specific helpers then adding the scoped_guard()
around the switch statement.

But really this is a question of taste and avoiding unneeded code churn and
unneeded nested scopes. There is no one clear way, so please use your
judgement per case and we can debate on your choices in review.

Thanks,
Amir.