Re: [PATCH v4] overlay: test for whiteout inode sharing

From: Amir Goldstein <amir73il@gmail.com>
To: Chengguang Xu <cgxu519@mykernel.net>
Cc: Eryu Guan <eguan@linux.alibaba.com>, Eryu Guan <guan@eryu.me>,
	miklos <miklos@szeredi.hu>, fstests <fstests@vger.kernel.org>,
	linux-unionfs <linux-unionfs@vger.kernel.org>
Subject: Re: [PATCH v4] overlay: test for whiteout inode sharing
Date: Wed, 13 May 2020 06:37:52 +0300	[thread overview]
Message-ID: <CAOQ4uxhV4ubSLmwTh9dHg-FWXYHo8uMh8QVNXhmtN=ahBFRoHg@mail.gmail.com> (raw)
In-Reply-To: <1720c092a68.c8052b8d3001.6829163626760635444@mykernel.net>

On Wed, May 13, 2020 at 6:17 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期三, 2020-05-13 09:10:19 Eryu Guan <eguan@linux.alibaba.com> 撰写 ----
>  > On Tue, May 12, 2020 at 07:56:35PM +0300, Amir Goldstein wrote:
>  > > > >  > I see no feature detection logic, so test just fails on old kernels
>  > > > >  > without this feature? I tried with v5.7-r4 kernel, test fails because
>  > > > >  > each whiteout file has only one hardlink.
>  > > > >
>  > > > > That's true.
>  > > >
>  > > > I'd like to see it _notrun on old kernels where the feature is not
>  > > > available. But that seems hard to do.. Do you have any better ideas?
>  > > >
>  > >
>  > > I've got a few.
>  > > 1. LTP has the concept of require minimum kernel version.
>  > >     This would mean that functionality will be not be tested if feature
>  > >     is backported to old kernels.
>  > > 2. We could add to overlayfs advertising of supported features, like
>  > >      /sys/fs/ext4/features/, but it already does "advertise" the configurable
>  > >      features at  /sys/module/overlay/parameters/, and we were already
>  > >      asking the question during patch review:
>  > >         /* Is there a reason anyone would want not to share whiteouts? */
>  > >         ofs->share_whiteout = true;
>  > >      and we left the answer to "later" time.
>  > >
>  > > So a simple solution would be to add the module parameter (without adding
>  > > a mount option), because:
>  > > - It doesn't hurt (?)
>  > > - Somebody may end up using it, for some reason we did not think of
>  > > - We can use it in test to require the feature
>  >
>  > Yeah, I think that works. And I see that ext4 and btrfs both have a
>  > /sys/fs/<fs>/features directory and list supported features there, is
>  > this something overlay could do? Or is this basically the same thing as
>  > what you proposed?
>  >
>
> IMO, for those features which don't need to change module param, maybe feature list
> is more suitable.
>

I suppose it is more suitable, but since at the moment there is only one(?)
such feature and there is an open question whether it should or should not
be configurable, I myself would have taken the easy path, but Miklos
often has a different perspective on these sort of things...

Thanks,
Amir.