| Message ID | 1557379616-13850-1-git-send-email-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/530: fix shutdown failure of generic/530 in overlay | expand |
On Thu, May 09, 2019 at 01:26:56PM +0800, Jeffle Xu wrote: > We got "shutdown: Inappropriate ioctl for device" when running > "./check -overlay generic/530". The error message is due to the > XFS_IOC_GOINGDOWN ioctl used in generic/530. > > Though _require_scratch_shutdown() is called to test whether the > tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece > of C code snippet in src/t_open_tmpfiles.c is used to shutdown the > filesysetm, rather than calling the corresponding helper function > _scratch_shutdown(). > > Let me explain it clearly: > > 1. suppose the config is > > ``` > export TEST_DEV=/dev/vdb > export TEST_DIR=/mnt/test > export SCRATCH_DEV=/dev/vdc > export SCRATCH_MNT=/mnt/scratch > ``` > > 2. When running "./check -overlay generic/530", the scratch device, > /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is > mounted on /mnt/scratch/mnt using the follwing command > > ``` > mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ > -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ > overlay /mnt/scratch/ovl-mnt > ``` > > 3. In this case, _require_scratch_shutdown() will inspect whether the > underlying upper system, that is **/mnt/scratch/**, supports shutdown > feature or not. In my test, the underlying system of overlay (that is > /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown > test. > > 4. However, the C code executing the shutdown action in t_open_tmpfiles.c > actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay > filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support > XFS_IOC_GOINGDOWN ioctl, it fails naturally. > > 5. So we should use _scratch_shutdown() to shutdown if we have used > _require_scratch_shutdown > > As for the solution to fix the failure, I temporarily move the shutdown > action from t_open_tmpfiles into generic/530 as the workaround. How would > you think @Darrick? Maybe there is a better solution but I'm not sure. *OH*, it escaped my notice that _require_scratch_shutdown tests shutting down the legs that overlayfs stands on, without testing the overlayfs mount itself. Sorry about that, and thanks for pointing that out. Hmm, well, this test is really about creating a bunch of temp files on SCRATCH_MNT and then shutting down the *SCRATCH_MNT* to see how it deals with that. I'm not sure why the _require helper special-cases overlayfs to test something other than SCRATCH_MNT, so I think we (or I) need clarification about why the helper does that, or possibly just a new _require helper that actually checks SCRATCH_MNT itself. TLDR: I thought that if _require_scratch_shutdown didn't stop the test then it was ok for any program to use the shutdown ioctl on the scratch mount, and was surprised when it didn't do this for overlayfs. --D > thx > > Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> > --- > tests/generic/530 | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tests/generic/530 b/tests/generic/530 > index b0d188b..56c6d32 100755 > --- a/tests/generic/530 > +++ b/tests/generic/530 > @@ -49,7 +49,9 @@ ulimit -n $max_files > > # Open a lot of unlinked files > echo create >> $seqres.full > -$here/src/t_open_tmpfiles $SCRATCH_MNT shutdown >> $seqres.full > +$here/src/t_open_tmpfiles $SCRATCH_MNT >> $seqres.full > +_scratch_shutdown -f > + > > # Unmount to prove that we can clean it all > echo umount >> $seqres.full > -- > 1.8.3.1 >
On Thu, May 9, 2019 at 8:29 AM Jeffle Xu <jefflexu@linux.alibaba.com> wrote: > > We got "shutdown: Inappropriate ioctl for device" when running > "./check -overlay generic/530". The error message is due to the > XFS_IOC_GOINGDOWN ioctl used in generic/530. > > Though _require_scratch_shutdown() is called to test whether the > tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece > of C code snippet in src/t_open_tmpfiles.c is used to shutdown the > filesysetm, rather than calling the corresponding helper function > _scratch_shutdown(). > > Let me explain it clearly: > > 1. suppose the config is > > ``` > export TEST_DEV=/dev/vdb > export TEST_DIR=/mnt/test > export SCRATCH_DEV=/dev/vdc > export SCRATCH_MNT=/mnt/scratch > ``` > > 2. When running "./check -overlay generic/530", the scratch device, > /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is > mounted on /mnt/scratch/mnt using the follwing command > > ``` > mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ > -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ > overlay /mnt/scratch/ovl-mnt > ``` > > 3. In this case, _require_scratch_shutdown() will inspect whether the > underlying upper system, that is **/mnt/scratch/**, supports shutdown > feature or not. In my test, the underlying system of overlay (that is > /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown > test. > > 4. However, the C code executing the shutdown action in t_open_tmpfiles.c > actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay > filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support > XFS_IOC_GOINGDOWN ioctl, it fails naturally. > > 5. So we should use _scratch_shutdown() to shutdown if we have used > _require_scratch_shutdown > > As for the solution to fix the failure, I temporarily move the shutdown > action from t_open_tmpfiles into generic/530 as the workaround. How would > you think @Darrick? Maybe there is a better solution but I'm not sure. > I think this is the correct solution (and not a workaround) FWIW there are other operations (e.g. umount) when done directly and not via common helpers would not have the same result when running check -overlay. I suggest that you remove the option 'shutdown' from t_open_tmpfiles if shutdown can be done by test, so future tests won't use it. There are 2 generic tests that use src/godown directly and not via common shutdown helper. Those tests are skipped with -overlay due to other unmet requirements, but as a general practice, using src/godown directly should be avoided if possible. Thanks, Amir.
On Thu, May 9, 2019 at 8:45 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, May 09, 2019 at 01:26:56PM +0800, Jeffle Xu wrote: > > We got "shutdown: Inappropriate ioctl for device" when running > > "./check -overlay generic/530". The error message is due to the > > XFS_IOC_GOINGDOWN ioctl used in generic/530. > > > > Though _require_scratch_shutdown() is called to test whether the > > tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece > > of C code snippet in src/t_open_tmpfiles.c is used to shutdown the > > filesysetm, rather than calling the corresponding helper function > > _scratch_shutdown(). > > > > Let me explain it clearly: > > > > 1. suppose the config is > > > > ``` > > export TEST_DEV=/dev/vdb > > export TEST_DIR=/mnt/test > > export SCRATCH_DEV=/dev/vdc > > export SCRATCH_MNT=/mnt/scratch > > ``` > > > > 2. When running "./check -overlay generic/530", the scratch device, > > /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is > > mounted on /mnt/scratch/mnt using the follwing command > > > > ``` > > mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ > > -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ > > overlay /mnt/scratch/ovl-mnt > > ``` > > > > 3. In this case, _require_scratch_shutdown() will inspect whether the > > underlying upper system, that is **/mnt/scratch/**, supports shutdown > > feature or not. In my test, the underlying system of overlay (that is > > /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown > > test. > > > > 4. However, the C code executing the shutdown action in t_open_tmpfiles.c > > actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay > > filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support > > XFS_IOC_GOINGDOWN ioctl, it fails naturally. > > > > 5. So we should use _scratch_shutdown() to shutdown if we have used > > _require_scratch_shutdown > > > > As for the solution to fix the failure, I temporarily move the shutdown > > action from t_open_tmpfiles into generic/530 as the workaround. How would > > you think @Darrick? Maybe there is a better solution but I'm not sure. > > *OH*, it escaped my notice that _require_scratch_shutdown tests shutting > down the legs that overlayfs stands on, without testing the overlayfs > mount itself. Sorry about that, and thanks for pointing that out. > > Hmm, well, this test is really about creating a bunch of temp files on > SCRATCH_MNT and then shutting down the *SCRATCH_MNT* to see how it deals > with that. I'm not sure why the _require helper special-cases overlayfs > to test something other than SCRATCH_MNT, so I think we (or I) need > clarification about why the helper does that, or possibly just a new > _require helper that actually checks SCRATCH_MNT itself. > The helper is doing the right thing as far as the tests that use the common shutdown helpers are concerned. When you yank the cord out of underlying fs, you effectively yank the cord out of overlayfs, so shutdown tests can be run on overlay thanks to the fixes made by Chengguang, helping with crash test coverage for overlayfs. From quick grep I found only 3 generic tests that issue goingdown via binaries and not via common helper. This patch fixes one of them and the other 2 tests (using src/godown) do not run on overlays, so it is low priority to fix them. Cheers, Amir.
CC uptodate email of Chengguang On Thu, May 9, 2019 at 12:07 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Thu, May 9, 2019 at 8:45 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Thu, May 09, 2019 at 01:26:56PM +0800, Jeffle Xu wrote: > > > We got "shutdown: Inappropriate ioctl for device" when running > > > "./check -overlay generic/530". The error message is due to the > > > XFS_IOC_GOINGDOWN ioctl used in generic/530. > > > > > > Though _require_scratch_shutdown() is called to test whether the > > > tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece > > > of C code snippet in src/t_open_tmpfiles.c is used to shutdown the > > > filesysetm, rather than calling the corresponding helper function > > > _scratch_shutdown(). > > > > > > Let me explain it clearly: > > > > > > 1. suppose the config is > > > > > > ``` > > > export TEST_DEV=/dev/vdb > > > export TEST_DIR=/mnt/test > > > export SCRATCH_DEV=/dev/vdc > > > export SCRATCH_MNT=/mnt/scratch > > > ``` > > > > > > 2. When running "./check -overlay generic/530", the scratch device, > > > /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is > > > mounted on /mnt/scratch/mnt using the follwing command > > > > > > ``` > > > mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ > > > -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ > > > overlay /mnt/scratch/ovl-mnt > > > ``` > > > > > > 3. In this case, _require_scratch_shutdown() will inspect whether the > > > underlying upper system, that is **/mnt/scratch/**, supports shutdown > > > feature or not. In my test, the underlying system of overlay (that is > > > /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown > > > test. > > > > > > 4. However, the C code executing the shutdown action in t_open_tmpfiles.c > > > actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay > > > filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support > > > XFS_IOC_GOINGDOWN ioctl, it fails naturally. > > > > > > 5. So we should use _scratch_shutdown() to shutdown if we have used > > > _require_scratch_shutdown > > > > > > As for the solution to fix the failure, I temporarily move the shutdown > > > action from t_open_tmpfiles into generic/530 as the workaround. How would > > > you think @Darrick? Maybe there is a better solution but I'm not sure. > > > > *OH*, it escaped my notice that _require_scratch_shutdown tests shutting > > down the legs that overlayfs stands on, without testing the overlayfs > > mount itself. Sorry about that, and thanks for pointing that out. > > > > Hmm, well, this test is really about creating a bunch of temp files on > > SCRATCH_MNT and then shutting down the *SCRATCH_MNT* to see how it deals > > with that. I'm not sure why the _require helper special-cases overlayfs > > to test something other than SCRATCH_MNT, so I think we (or I) need > > clarification about why the helper does that, or possibly just a new > > _require helper that actually checks SCRATCH_MNT itself. > > > > The helper is doing the right thing as far as the tests that use the common > shutdown helpers are concerned. > When you yank the cord out of underlying fs, you effectively yank the cord > out of overlayfs, so shutdown tests can be run on overlay thanks to the fixes > made by Chengguang, helping with crash test coverage for overlayfs. > > From quick grep I found only 3 generic tests that issue goingdown via > binaries and not via common helper. This patch fixes one of them and the > other 2 tests (using src/godown) do not run on overlays, so it is low > priority to fix them. > > Cheers, > Amir.
On Thu, May 09, 2019 at 11:59:31AM +0300, Amir Goldstein wrote: > On Thu, May 9, 2019 at 8:29 AM Jeffle Xu <jefflexu@linux.alibaba.com> wrote: > > > > We got "shutdown: Inappropriate ioctl for device" when running > > "./check -overlay generic/530". The error message is due to the > > XFS_IOC_GOINGDOWN ioctl used in generic/530. > > > > Though _require_scratch_shutdown() is called to test whether the > > tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece > > of C code snippet in src/t_open_tmpfiles.c is used to shutdown the > > filesysetm, rather than calling the corresponding helper function > > _scratch_shutdown(). > > > > Let me explain it clearly: > > > > 1. suppose the config is > > > > ``` > > export TEST_DEV=/dev/vdb > > export TEST_DIR=/mnt/test > > export SCRATCH_DEV=/dev/vdc > > export SCRATCH_MNT=/mnt/scratch > > ``` > > > > 2. When running "./check -overlay generic/530", the scratch device, > > /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is > > mounted on /mnt/scratch/mnt using the follwing command > > > > ``` > > mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ > > -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ > > overlay /mnt/scratch/ovl-mnt > > ``` > > > > 3. In this case, _require_scratch_shutdown() will inspect whether the > > underlying upper system, that is **/mnt/scratch/**, supports shutdown > > feature or not. In my test, the underlying system of overlay (that is > > /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown > > test. > > > > 4. However, the C code executing the shutdown action in t_open_tmpfiles.c > > actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay > > filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support > > XFS_IOC_GOINGDOWN ioctl, it fails naturally. > > > > 5. So we should use _scratch_shutdown() to shutdown if we have used > > _require_scratch_shutdown > > > > As for the solution to fix the failure, I temporarily move the shutdown > > action from t_open_tmpfiles into generic/530 as the workaround. How would > > you think @Darrick? Maybe there is a better solution but I'm not sure. > > > > I think this is the correct solution (and not a workaround) > FWIW there are other operations (e.g. umount) when done directly and > not via common helpers would not have the same result when running > check -overlay. I agreed. > > I suggest that you remove the option 'shutdown' from t_open_tmpfiles > if shutdown can be done by test, so future tests won't use it. > > There are 2 generic tests that use src/godown directly and not via > common shutdown helper. Those tests are skipped with -overlay due > to other unmet requirements, but as a general practice, using src/godown > directly should be avoided if possible. Yeah, makes sense to me, thanks! Eryu
diff --git a/tests/generic/530 b/tests/generic/530 index b0d188b..56c6d32 100755 --- a/tests/generic/530 +++ b/tests/generic/530 @@ -49,7 +49,9 @@ ulimit -n $max_files # Open a lot of unlinked files echo create >> $seqres.full -$here/src/t_open_tmpfiles $SCRATCH_MNT shutdown >> $seqres.full +$here/src/t_open_tmpfiles $SCRATCH_MNT >> $seqres.full +_scratch_shutdown -f + # Unmount to prove that we can clean it all echo umount >> $seqres.full
We got "shutdown: Inappropriate ioctl for device" when running "./check -overlay generic/530". The error message is due to the XFS_IOC_GOINGDOWN ioctl used in generic/530. Though _require_scratch_shutdown() is called to test whether the tested filesystem supports XFS_IOC_GOINGDOWN ioctl or not, a piece of C code snippet in src/t_open_tmpfiles.c is used to shutdown the filesysetm, rather than calling the corresponding helper function _scratch_shutdown(). Let me explain it clearly: 1. suppose the config is ``` export TEST_DEV=/dev/vdb export TEST_DIR=/mnt/test export SCRATCH_DEV=/dev/vdc export SCRATCH_MNT=/mnt/scratch ``` 2. When running "./check -overlay generic/530", the scratch device, /dev/vdc is mounted on /mnt/scratch, and the overlay filesystem is mounted on /mnt/scratch/mnt using the follwing command ``` mount -t overlay -o lowerdir=/mnt/scratch/ovl-lower \ -o upperdir=/mnt/scratch/ovl-upper/ -o workdir=/mnt/scratch/ovl-work/ \ overlay /mnt/scratch/ovl-mnt ``` 3. In this case, _require_scratch_shutdown() will inspect whether the underlying upper system, that is **/mnt/scratch/**, supports shutdown feature or not. In my test, the underlying system of overlay (that is /dev/vdc) is ext4, and thus it passes the _require_scratch_shutdown test. 4. However, the C code executing the shutdown action in t_open_tmpfiles.c actually execute XFS_IOC_GOINGDOWN ioctl on the mount point of overlay filesystem, that is **/mnt/scratch/ovl-mnt**. Since overlay doesn't support XFS_IOC_GOINGDOWN ioctl, it fails naturally. 5. So we should use _scratch_shutdown() to shutdown if we have used _require_scratch_shutdown As for the solution to fix the failure, I temporarily move the shutdown action from t_open_tmpfiles into generic/530 as the workaround. How would you think @Darrick? Maybe there is a better solution but I'm not sure. thx Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> --- tests/generic/530 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)