| Message ID | 1391703008-2322-1-git-send-email-wangshilong1991@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 07, 2014 at 12:10:08AM +0800, Wang Shilong wrote: > +$BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT \ > + $SCRATCH_MNT/snap_1 >> $seqres.full 2>&1 > + > +do_snapshots & > +snapshots_pid=$! > + > +$BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 > /dev/null 2>&1 || echo "btrfs send failed" Let's stop this anti-pattern before it takes hold. If there's output from the send command it should be filtered and captured in the golden image. Hence any deviation caused by errors is automatically flagged as an error. That's the whole point of using golden images for capturing errors - you don't need to capture return values from binaries and it guarantees that users are informed about failures through error messages. IOWs: $BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 | _btrfs_send_filter is what you should be doing here. Cheers, Dave.
Hi Dave, > On Fri, Feb 07, 2014 at 12:10:08AM +0800, Wang Shilong wrote: >> +$BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT \ >> + $SCRATCH_MNT/snap_1 >> $seqres.full 2>&1 >> + >> +do_snapshots & >> +snapshots_pid=$! >> + >> +$BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 > /dev/null 2>&1 || echo "btrfs send failed" > > Let's stop this anti-pattern before it takes hold. > > If there's output from the send command it should be > filtered and captured in the golden image. Hence any deviation > caused by errors is automatically flagged as an error. > > That's the whole point of using golden images for capturing errors - > you don't need to capture return values from binaries and it > guarantees that users are informed about failures through error > messages. IOWs: > > $BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 | _btrfs_send_filter > > is what you should be doing here. I knew what you mean here, in fact, i did this on purpose. for this test failure, btrfs-prog did not output failure information from the beginning. So to make older progs can also detect the test failure, i dropped into this way. Anyway, if you don't like this and think old btrfs-progs needn't consider this, i will update the patch.^_^ Thanks, Wang > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 07, 2014 at 12:18:31PM +0800, Wang Shilong wrote: > > Hi Dave, > > > On Fri, Feb 07, 2014 at 12:10:08AM +0800, Wang Shilong wrote: > >> +$BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT \ > >> + $SCRATCH_MNT/snap_1 >> $seqres.full 2>&1 > >> + > >> +do_snapshots & > >> +snapshots_pid=$! > >> + > >> +$BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 > /dev/null 2>&1 || echo "btrfs send failed" > > > > Let's stop this anti-pattern before it takes hold. > > > > If there's output from the send command it should be > > filtered and captured in the golden image. Hence any deviation > > caused by errors is automatically flagged as an error. > > > > That's the whole point of using golden images for capturing errors - > > you don't need to capture return values from binaries and it > > guarantees that users are informed about failures through error > > messages. IOWs: > > > > $BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 | _btrfs_send_filter > > > > is what you should be doing here. > > I knew what you mean here, in fact, i did this on purpose. Ok, then you need to explain why you did it on purpose with a comment. It's just as important to explain the reason for doing something in test code as it is in the kernel code. i.e. so when we are looking at the test in 5 years time we know the reason for it being that way. > for this test failure, btrfs-prog did not output failure > information from the beginning. I have nothing good to say about that state of affairs, but... > So to make older progs can also > detect the test failure, i dropped into this way. .. it's going to have to stay like it. Please insert an appropriately sarcastic comment about the usefulness of a silent send command here, because if I write it I'm going to offend lots of people. :/ Cheers, Dave.
Hi Dave, > On Fri, Feb 07, 2014 at 12:18:31PM +0800, Wang Shilong wrote: >> >> Hi Dave, >> >>> On Fri, Feb 07, 2014 at 12:10:08AM +0800, Wang Shilong wrote: >>>> +$BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT \ >>>> + $SCRATCH_MNT/snap_1 >> $seqres.full 2>&1 >>>> + >>>> +do_snapshots & >>>> +snapshots_pid=$! >>>> + >>>> +$BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 > /dev/null 2>&1 || echo "btrfs send failed" >>> >>> Let's stop this anti-pattern before it takes hold. >>> >>> If there's output from the send command it should be >>> filtered and captured in the golden image. Hence any deviation >>> caused by errors is automatically flagged as an error. >>> >>> That's the whole point of using golden images for capturing errors - >>> you don't need to capture return values from binaries and it >>> guarantees that users are informed about failures through error >>> messages. IOWs: >>> >>> $BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 | _btrfs_send_filter >>> >>> is what you should be doing here. >> >> I knew what you mean here, in fact, i did this on purpose. > > Ok, then you need to explain why you did it on purpose with a comment. > It's just as important to explain the reason for doing something in > test code as it is in the kernel code. i.e. so when we are looking > at the test in 5 years time we know the reason for it being that > way. > >> for this test failure, btrfs-prog did not output failure >> information from the beginning. > > I have nothing good to say about that state of affairs, but... > >> So to make older progs can also >> detect the test failure, i dropped into this way. > > .. it's going to have to stay like it. Please insert an > appropriately sarcastic comment about the usefulness of a silent > send command here, because if I write it I'm going to offend lots of > people. :/ Sorry, my miss, when i was going to give a patch for btrfs-progs, i noticed the issue has been fixed in latest btrfs-progs(3.12 which has released for a long time). So let's drop the way you have said before. Thanks, Wang > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tests/btrfs/034 b/tests/btrfs/034 new file mode 100644 index 0000000..5152969 --- /dev/null +++ b/tests/btrfs/034 @@ -0,0 +1,83 @@ +#!/bin/bash +# FS QA Test No. btrfs/034 +# +# Regression test for running snapshots and send concurrently. +# +#----------------------------------------------------------------------- +# Copyright (c) 2014 Fujitsu. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +# +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -f $tmp.* +} + +do_snapshots() +{ + i=2 + while [ $i -lt 50 ] + do + $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/snap_1 \ + $SCRATCH_MNT/snap_$i >> $seqres.full 2>&1 + let i=$i+1 + done +} + +trap "_cleanup ; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_fs btrfs +_supported_os Linux +_require_scratch + +_scratch_mkfs > /dev/null 2>&1 +_scratch_mount + + +touch $SCRATCH_MNT/foo + +# get file with fragments by using backwards writes. +for i in `seq 10240 -1 1`; do + $XFS_IO_PROG -f -d -c "pwrite $(($i * 4096)) 4096" \ + $SCRATCH_MNT/foo > /dev/null | _filter_xfs_io +done + +$BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT \ + $SCRATCH_MNT/snap_1 >> $seqres.full 2>&1 + +do_snapshots & +snapshots_pid=$! + +$BTRFS_UTIL_PROG send $SCRATCH_MNT/snap_1 > /dev/null 2>&1 || echo "btrfs send failed" + +kill -TERM $snapshots_pid 2> /dev/null + +echo "Silence is golden" +status=0 ; exit diff --git a/tests/btrfs/034.out b/tests/btrfs/034.out new file mode 100644 index 0000000..4c8873c --- /dev/null +++ b/tests/btrfs/034.out @@ -0,0 +1,2 @@ +QA output created by 034 +Silence is golden diff --git a/tests/btrfs/group b/tests/btrfs/group index b29236c..f9f062f 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -36,3 +36,4 @@ 031 auto quick 032 auto quick 033 auto quick +034 auto quick