public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
@ 2012-07-30 12:06 Tomas Racek
  2012-07-30 22:04 ` Dave Chinner
  0 siblings, 1 reply; 6+ messages in thread
From: Tomas Racek @ 2012-07-30 12:06 UTC (permalink / raw)
  To: xfs; +Cc: lczerner, Tomas Racek

Rename fstrim option from "-s" to "-o" and change output message when
verbose option is set.

Signed-off-by: Tomas Racek <tracek@redhat.com>
---
 251          |    2 +-
 260          |   22 ++++++++++++----------
 src/fstrim.c |   11 ++++++-----
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/251 b/251
index f46b6e2..a70798e 100755
--- a/251
+++ b/251
@@ -107,7 +107,7 @@ fstrim_loop()
 			wait $fpid
 		fi
 		while [ $start -lt $fsize ] ; do
-			$here/src/fstrim -m ${minlen}k -s ${start}k -l ${step}k $SCRATCH_MNT &
+			$here/src/fstrim -m ${minlen}k -o ${start}k -l ${step}k $SCRATCH_MNT &
 			fpid=$!
 			wait $fpid
 			start=$(( $start + $step ))
diff --git a/260 b/260
index b005cd3..52527da 100755
--- a/260
+++ b/260
@@ -59,19 +59,19 @@ max_64bit=$(_math "2^64 - 1")
 # the file system
 
 echo "[+] Start beyond the end of fs (should fail)"
-"$FSTRIM" -s $beyond_eofs $SCRATCH_MNT
+"$FSTRIM" -o $beyond_eofs $SCRATCH_MNT
 [ $? -eq 0 ] && status=1
 
 echo "[+] Start beyond the end of fs with len set (should fail)"
-"$FSTRIM" -s $beyond_eofs -l1M $SCRATCH_MNT
+"$FSTRIM" -o $beyond_eofs -l1M $SCRATCH_MNT
 [ $? -eq 0 ] && status=1
 
 echo "[+] Start = 2^64-1 (should fail)"
-"$FSTRIM" -s $max_64bit $SCRATCH_MNT
+"$FSTRIM" -o $max_64bit $SCRATCH_MNT
 [ $? -eq 0 ] && status=1
 
 echo "[+] Start = 2^64-1 and len is set (should fail)"
-"$FSTRIM" -s $max_64bit -l1M $SCRATCH_MNT
+"$FSTRIM" -o $max_64bit -l1M $SCRATCH_MNT
 [ $? -eq 0 ] && status=1
 
 _scratch_unmount
@@ -85,13 +85,13 @@ echo "[+] Default length (should succeed)"
 "$FSTRIM" $SCRATCH_MNT
 [ $? -ne 0 ] && status=1
 echo "[+] Default length with start set (should succeed)"
-"$FSTRIM" -s10M $SCRATCH_MNT
+"$FSTRIM" -o10M $SCRATCH_MNT
 [ $? -ne 0 ] && status=1
 echo "[+] Length beyond the end of fs (should succeed)"
 "$FSTRIM" -l $beyond_eofs $SCRATCH_MNT
 [ $? -ne 0 ] && status=1
 echo "[+] Length beyond the end of fs with start set (should succeed)"
-"$FSTRIM" -s10M -l $beyond_eofs $SCRATCH_MNT
+"$FSTRIM" -o10M -l $beyond_eofs $SCRATCH_MNT
 [ $? -ne 0 ] && status=1
 
 _scratch_unmount
@@ -101,8 +101,9 @@ _scratch_mount
 # This is a bit fuzzy, but since the file system is fresh
 # there should be at least (fssize/2) free space to trim.
 # This is supposed to catch wrong FITRIM argument handling
-out=$("$FSTRIM" -v -s10M $SCRATCH_MNT)
-bytes=${out%% *}
+out=$("$FSTRIM" -v -o10M $SCRATCH_MNT)
+nopref=${out##*: }
+bytes=${nopref%% *}
 
 if [ $bytes -gt $(_math "$fssize*1024") ]; then
 	status=1
@@ -155,7 +156,7 @@ _scratch_unmount
 _scratch_mkfs >/dev/null 2>&1
 _scratch_mount
 # It should fail since $start is beyond the end of file system
-"$FSTRIM" -s$start -l10M $SCRATCH_MNT &> /dev/null
+"$FSTRIM" -o$start -l10M $SCRATCH_MNT &> /dev/null
 if [ $? -eq 0 ]; then
 	status=1
 	echo "It seems that fs logic handling start"\
@@ -174,7 +175,8 @@ _scratch_mount
 # mapped and since we got here right after the mkfs, there is not
 # enough free extents in the root tree.
 out=$("$FSTRIM" -v -l$len $SCRATCH_MNT)
-bytes=${out%% *}
+nopref=${out##*: }
+bytes=${nopref%% *}
 if [ $bytes -le $(_math "$fssize*512") ] && [ $FSTYP != "btrfs" ]; then
 	status=1
 	echo "It seems that fs logic handling len argument overflows"
diff --git a/src/fstrim.c b/src/fstrim.c
index e23bcb3..9986d4e 100644
--- a/src/fstrim.c
+++ b/src/fstrim.c
@@ -58,9 +58,9 @@ struct options {
 static void usage(void)
 {
 	fprintf(stderr,
-		"Usage: %s [-s start] [-l length] [-m minimum-extent]"
+		"Usage: %s [-o offset] [-l length] [-m minimum-extent]"
 		" [-v] {mountpoint}\n\t"
-		"-s Starting Byte to discard from\n\t"
+		"-o Offset in Bytes to discard from\n\t"
 		"-l Number of Bytes to discard from the start\n\t"
 		"-m Minimum extent length to discard\n\t"
 		"-v Verbose - number of discarded bytes\n",
@@ -152,9 +152,9 @@ static int parse_opts(int argc, char **argv, struct options *opts)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "s:l:m:v")) != EOF) {
+	while ((c = getopt(argc, argv, "o:l:m:v")) != EOF) {
 		switch (c) {
-		case 's': /* starting point */
+		case 'o': /* starting point */
 			opts->range->start = get_number(&optarg);
 			break;
 		case 'l': /* length */
@@ -242,7 +242,8 @@ int main(int argc, char **argv)
 	}
 
 	if ((opts->verbose) && (opts->range))
-		fprintf(stdout, "%llu Bytes were trimmed\n", (unsigned long long)opts->range->len);
+		fprintf(stdout, "%s: %llu bytes were trimmed\n", opts->mpoint,
+			(unsigned long long)opts->range->len);
 
 	ret = EXIT_SUCCESS;
 
-- 
1.7.7.6

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
  2012-07-30 12:06 [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version Tomas Racek
@ 2012-07-30 22:04 ` Dave Chinner
  2012-07-31  2:33   ` Christoph Hellwig
  2012-07-31 10:24   ` Tomas Racek
  0 siblings, 2 replies; 6+ messages in thread
From: Dave Chinner @ 2012-07-30 22:04 UTC (permalink / raw)
  To: Tomas Racek; +Cc: lczerner, xfs

On Mon, Jul 30, 2012 at 02:06:03PM +0200, Tomas Racek wrote:
> Rename fstrim option from "-s" to "-o" and change output message when
> verbose option is set.

Why modify a local utility program like this?  There's little point
in trying to match some other utilities' command line parameters
just for the sake of it...

If we have duplicate code (i.e. a copy of the upstream utility) or
the local tool can be completely replaced by the upstream tool,
then we should use upstream and remove the local copy completely.
Distros have been shipping fstrim for long enough now that most
people running testing on upstream kernels will have it installed...

Adding a _require_fstrim() function that checks for the upstream
version of fstrim to be installed for each test that requires it
would go along with this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
  2012-07-30 22:04 ` Dave Chinner
@ 2012-07-31  2:33   ` Christoph Hellwig
  2012-07-31 12:01     ` Lukáš Czerner
  2012-07-31 10:24   ` Tomas Racek
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2012-07-31  2:33 UTC (permalink / raw)
  To: Dave Chinner; +Cc: lczerner, Tomas Racek, xfs

On Tue, Jul 31, 2012 at 08:04:13AM +1000, Dave Chinner wrote:
> If we have duplicate code (i.e. a copy of the upstream utility) or
> the local tool can be completely replaced by the upstream tool,
> then we should use upstream and remove the local copy completely.
> Distros have been shipping fstrim for long enough now that most
> people running testing on upstream kernels will have it installed...
> 
> Adding a _require_fstrim() function that checks for the upstream
> version of fstrim to be installed for each test that requires it
> would go along with this.

I would also vote for just using the upstream util-linux fstrim.  Not
quite sure what the history was here, but it might have been that the
xfstests one actually was the earlier version.  Lukas, any opinions?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
  2012-07-30 22:04 ` Dave Chinner
  2012-07-31  2:33   ` Christoph Hellwig
@ 2012-07-31 10:24   ` Tomas Racek
  2012-07-31 12:15     ` Lukáš Czerner
  1 sibling, 1 reply; 6+ messages in thread
From: Tomas Racek @ 2012-07-31 10:24 UTC (permalink / raw)
  To: Dave Chinner; +Cc: lczerner, xfs

> If we have duplicate code (i.e. a copy of the upstream utility) or
> the local tool can be completely replaced by the upstream tool,
> then we should use upstream and remove the local copy completely.
> Distros have been shipping fstrim for long enough now that most
> people running testing on upstream kernels will have it installed...
> 

OK, I'll create the patch which drops local version.

> Adding a _require_fstrim() function that checks for the upstream
> version of fstrim to be installed for each test that requires it
> would go along with this.

Did you mean something like

_require_fstrim()
{
         which fstrim &>/dev/null || _notrun "This test requires fstrim utility."
}

in common.rc or locally in each test?

Thanks for comments!

Tomas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
  2012-07-31  2:33   ` Christoph Hellwig
@ 2012-07-31 12:01     ` Lukáš Czerner
  0 siblings, 0 replies; 6+ messages in thread
From: Lukáš Czerner @ 2012-07-31 12:01 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: lczerner, Tomas Racek, xfs

On Mon, 30 Jul 2012, Christoph Hellwig wrote:

> Date: Mon, 30 Jul 2012 22:33:36 -0400
> From: Christoph Hellwig <hch@infradead.org>
> To: Dave Chinner <david@fromorbit.com>
> Cc: Tomas Racek <tracek@redhat.com>, lczerner@redhat.com, xfs@oss.sgi.com
> Subject: Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with
>     upstream version
> 
> On Tue, Jul 31, 2012 at 08:04:13AM +1000, Dave Chinner wrote:
> > If we have duplicate code (i.e. a copy of the upstream utility) or
> > the local tool can be completely replaced by the upstream tool,
> > then we should use upstream and remove the local copy completely.
> > Distros have been shipping fstrim for long enough now that most
> > people running testing on upstream kernels will have it installed...
> > 
> > Adding a _require_fstrim() function that checks for the upstream
> > version of fstrim to be installed for each test that requires it
> > would go along with this.
> 
> I would also vote for just using the upstream util-linux fstrim.  Not
> quite sure what the history was here, but it might have been that the
> xfstests one actually was the earlier version.  Lukas, any opinions?
> 

The local xfstests version was indeed the earlier version and it was
not even in the util-linux back then. So now, when we already have
fstrim in util-linux and most distributions already ship it, I do
not see any reason for maintaining the local copy anymore.

I agree that we should be using upstream fstrim and remove the local
version completely.

Thanks!
-Lukas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version
  2012-07-31 10:24   ` Tomas Racek
@ 2012-07-31 12:15     ` Lukáš Czerner
  0 siblings, 0 replies; 6+ messages in thread
From: Lukáš Czerner @ 2012-07-31 12:15 UTC (permalink / raw)
  To: Tomas Racek; +Cc: lczerner, xfs

On Tue, 31 Jul 2012, Tomas Racek wrote:

> Date: Tue, 31 Jul 2012 06:24:26 -0400 (EDT)
> From: Tomas Racek <tracek@redhat.com>
> To: Dave Chinner <david@fromorbit.com>
> Cc: lczerner@redhat.com, xfs@oss.sgi.com
> Subject: Re: [PATCH] xfstests: Change fstrim behaviour to be consistent with
>     upstream version
> 
> > If we have duplicate code (i.e. a copy of the upstream utility) or
> > the local tool can be completely replaced by the upstream tool,
> > then we should use upstream and remove the local copy completely.
> > Distros have been shipping fstrim for long enough now that most
> > people running testing on upstream kernels will have it installed...
> > 
> 
> OK, I'll create the patch which drops local version.
> 
> > Adding a _require_fstrim() function that checks for the upstream
> > version of fstrim to be installed for each test that requires it
> > would go along with this.
> 
> Did you mean something like
> 
> _require_fstrim()
> {
>          which fstrim &>/dev/null || _notrun "This test requires fstrim utility."
> }
> 
> in common.rc or locally in each test?

I think that having this test in common.rc along with others is definitely
better option.

And while you're in it, you can also add another _require_ for the
actual FITRIM support. Although calling it _require_fitrim seems
rather confusing, so maybe _require_batched_discard with the device
as an argument ?

Thanks!
-Lukas

> 
> Thanks for comments!
> 
> Tomas
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-07-31 12:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-30 12:06 [PATCH] xfstests: Change fstrim behaviour to be consistent with upstream version Tomas Racek
2012-07-30 22:04 ` Dave Chinner
2012-07-31  2:33   ` Christoph Hellwig
2012-07-31 12:01     ` Lukáš Czerner
2012-07-31 10:24   ` Tomas Racek
2012-07-31 12:15     ` Lukáš Czerner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox