On Tue, Oct 18, 2016 at 10:53 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > On Tue, Oct 18, 2016 at 9:41 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> On Fri, Sep 16, 2016 at 3:19 PM, Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote: >>> The problem with writecount is: we want consistent handling of it for >>> underlying filesystems as well as overlayfs. Making sure i_writecount is >>> correct on all layers is difficult. Instead this patch makes sure that >>> when write access is acquired, it's always done on the underlying writable >>> layer (called the upper layer). We must also make sure to look at the >>> writecount on this layer when checking for conflicting leases. >>> >>> Open for write already updates the upper layer's writecount. Leaving only >>> truncate. >>> >>> For truncate copy up must happen before get_write_access() so that the >>> writecount is updated on the upper layer. Problem with this is if >>> something fails after that, then copy-up was done needlessly. E.g. if >>> break_lease() was interrupted. Probably not a big deal in practice. >>> >>> Another interesting case is if there's a denywrite on a lower file that is >>> then opened for write or truncated. With this patch these will succeed, >>> which is somewhat counterintuitive. But I think it's still acceptable, >>> considering that the copy-up does actually create a different file, so the >>> old, denywrite mapping won't be touched. >> >> Miklos, >> I think this breaks xfstest overlay/013 on v4.8, because execve() does >> deny write on lower inode and then truncate happens on upper inode. > > It does break the xfstest, but as explained in the patch it shouldn't > be a problem in practice. I think the test should just be fixed. > Right. and so I did fix the test, which had a bug anyway and was not testing exec+truncate from upper at all. But I noticed a strange behavior: When you run t_truncate_self from upper you get success for not being able to truncate. When you run t_truncate_self from lower you get an error for being able to truncate This is ok as you write, but... When you re-run t_truncate_self file that just got copied-up you get segmentation fault and that is not ok. You get try the patch to xfstest below to reproduce the problem: diff --git a/tests/overlay/013 b/tests/overlay/013 index 09f3eb1..cd3d0c0 100755 --- a/tests/overlay/013 +++ b/tests/overlay/013 @@ -59,7 +59,7 @@ upperdir=$SCRATCH_DEV/$OVERLAY_UPPER_DIR mkdir -p $lowerdir mkdir -p $upperdir cp $here/src/t_truncate_self $lowerdir/test_lower -cp $here/src/t_truncate_self $lowerdir/test_upper +cp $here/src/t_truncate_self $upperdir/test_upper _scratch_mount @@ -67,8 +67,9 @@ _scratch_mount # test programs truncate themselfs, all should fail with ETXTBSY $SCRATCH_MNT/test_lower $SCRATCH_MNT/test_upper +# run test again after copy-up +$SCRATCH_MNT/test_lower # success, all done -echo "Silence is golden" status=0 exit diff --git a/tests/overlay/013.out b/tests/overlay/013.out index 3e66423..a8ee7cd 100644 --- a/tests/overlay/013.out +++ b/tests/overlay/013.out @@ -1,2 +1,2 @@ QA output created by 013 -Silence is golden +truncate(argv[0]) should have failed -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
