NACK: [SRU][UNSTABLE/FOCAL/BIONIC][PATCH v2] UBUNTU: SAUCE: shiftfs: fix dentry revalidation

Stefan Bader stefan.bader at canonical.com
Tue May 19 08:52:10 UTC 2020


On 19.05.20 08:02, Christian Brauner wrote:
> On Tue, May 19, 2020 at 07:43:21AM +0200, Stefan Bader wrote:
>> On 18.05.20 20:18, Christian Brauner wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1879196
>>>
>>> While fixing [1] we introduced a regression and triggered [2] whereby
>>> the underlay and overlay go out of sync because they disagree about the
>>> dcache. Such situation easily arise when the underlay is modified
>>> directly. This means the overlay needs to revalidate the dentry in the
>>> dcache to catchup with the underlay.
>>>
>>> Note, in general it's not advisable to directly modify the underlay
>>> while a shiftfs mount is on top. In some way this means we need to keep
>>> two caches in sync and it's hard enough to keep a single cache happy.
>>> But shiftfs' use-case is inherently prone to be used for exactly that.
>>> So this is something we have to navigate carefully and honestly we have
>>> no full model upstream that does the same. Overlayfs has the copy-up
>>> behavior which let's it get around most of the issues but we don't have
>>> it and ecryptfs is broken in such scenarios which we verified quite a
>>> while back.
>>> In any case, I built a kernel with this patch and re-ran all regressions
>>> that are related to this that we have so far (cf.  [1], [2], and [3]).
>>> None of them were reproducible with this patch here. So we still fix the
>>> ESTALE issue but also keep underlay and overlay in sync.
>>
>> This test kernel should be provided to the bug reporter, so it can be confirmed
>> to fix the issue there as well.
> 
> See
> https://drive.google.com/open?id=19iTwaFSYNS95_I-gD_rvFoV9cMAfy6io
> Elmo is also Cced here. I don't have any other place I can upload this,
> I think.

Since it is James, you could use private-fileshare.c.c, otherwise people.c.c for
providing test kernels. The reason I would like to see the reporter (did not
look close enough to see its James initially, but even better) confirm the fix
is that while you did regression test against the known issues, this might
introduce something new. And there some unknown use pattern not directly aimed
at something adds value.

> 
>> But more importantly, you submitted this for Bionic and there is no shiftfs in
>> that release. Only Eoan. I am not convinced you tried at least to apply the
> 
> Yeah, sorry that's always my thing mixing up that shiftfs is in Eoan and
> not in Bionic.
> Similar to all the other patches, Eoan doesn't need a separate patch
> I've verified this on top of Eoan master-next that has all the other
> shiftfs patches:

Problem there is that this causes confusion as the bug report has no nominations
to cross-check either. And the SRU justification should go into the description
there. Also one gets a bit doubtful of how much care was taken with the fix if
the submission looks like it was hurried out.

> 
> ubuntu at amdgpu:~/linux/ubuntu/ubuntu-eoan$ git am ./0001-UBUNTU-SAUCE-shiftfs-fix-dentry-revalidation.patch
> Applying: UBUNTU: SAUCE: shiftfs: fix dentry revalidation
> 
> on top of
> 
> ubuntu at amdgpu:~/linux/ubuntu/ubuntu-eoan$ git am ./0001-UBUNTU-SAUCE-shiftfs-fix-dentry-revalidation.patch
> Applying: UBUNTU: SAUCE: shiftfs: fix dentry revalidation
> commit d53d4b6679bb89ae78787dca0fea951752e27d48 (upstream/master-next)
> Author: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> Date:   Fri May 15 13:03:42 2020 +0200
> 
>     UBUNTU: Ubuntu-5.3.0-54.48
> 
>     Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> 
> I'm not letting shiftfs across kernel versions go out of sync precisely
> (at least until we absolutely have to) so that patches can be applied
> trivially across releases so it's easier for all of you.

I trust that this is true for shiftfs.c but you might be using kernel functions
outside your driver domain which do not exist in older kernels.

I would say once we get additional test results by James and you refreshed the
bug report (I added nominations for focal and eoan, the importance I set to
medium as I understood you as this might be an uncommon use-case which triggers
it), then re-send your v2 for unstable,focal, and eoan.

-Stefan
> 
> Thanks!
> Christian
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200519/b9fb8731/attachment-0001.sig>


More information about the kernel-team mailing list