[PATCH]: e2fsprogs m_* self tests no longer pass

[PATCH]: e2fsprogs m_* self tests no longer pass

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Only display the "Discarding device blocks:" status bar if discard is actually implemented in the IO manager and in the kernel.  Otherwise, there is no correct test result that will work in all environments.  I'd prefer not to print an error message for the majority of devices that do not support discard, since this is confusing.

Signed-off-by: Andreas Dilger <adilger@dilger.ca>



[-- Attachment #2: e2fsprogs-discard.patch --]
[-- Type: application/octet-stream, Size: 1880 bytes --]

Allow mke2fs discard patches to pass regression tests.

Index: e2fsprogs/lib/ext2fs/unix_io.c
===================================================================
--- e2fsprogs.orig/lib/ext2fs/unix_io.c
+++ e2fsprogs/lib/ext2fs/unix_io.c
@@ -872,8 +872,11 @@ static errcode_t unix_discard(io_channel
 	range[1] = (__uint64_t)(count) * channel->block_size;
 
 	ret = ioctl(data->dev, BLKDISCARD, &range);
-	if (ret < 0)
+	if (ret < 0) {
+		if (errno == ENOTTY)
+			return EXT2_ET_UNIMPLEMENTED;
 		return errno;
+	}
 	return 0;
 #else
 	return EXT2_ET_UNIMPLEMENTED;
Index: e2fsprogs/misc/mke2fs.c
===================================================================
--- e2fsprogs.orig/misc/mke2fs.c
+++ e2fsprogs/misc/mke2fs.c
@@ -2010,29 +2010,37 @@ static int mke2fs_discard_device(ext2_fi
 	count *= (1024 * 1024);
 	count /= fs->blocksize;
 
-	ext2fs_numeric_progress_init(fs, &progress,
-				     _("Discarding device blocks: "),
-				     blocks);
 	while (cur < blocks) {
-		ext2fs_numeric_progress_update(fs, &progress, cur);
-
 		if (cur + count > blocks)
 			count = blocks - cur;
 
 		retval = io_channel_discard(fs->io, cur, count, fs->blocksize);
+		if (cur == 0) {
+			/* If discard is unimplemented skip the progress bar */
+			if (retval == EXT2_ET_UNIMPLEMENTED)
+				return retval;
+
+			ext2fs_numeric_progress_init(fs, &progress,
+						_("Discarding device blocks: "),
+						blocks);
+		}
+
+		ext2fs_numeric_progress_update(fs, &progress, cur);
+
 		if (retval)
 			break;
+
 		cur += count;
 	}
 
 	if (retval) {
-		ext2fs_numeric_progress_close(fs, &progress,
-				      _("failed - "));
+		ext2fs_numeric_progress_close(fs, &progress, _("failed - "));
 		if (!quiet)
 			printf("%s\n",error_message(retval));
-	} else
+	} else {
 		ext2fs_numeric_progress_close(fs, &progress,
 				      _("done                            \n"));
+	}
 
 	return retval;
 }

Re: [PATCH]: e2fsprogs m_* self tests no longer pass

[Lukas Czerner]

On Tue, 15 Mar 2011, Andreas Dilger wrote:

> Only display the "Discarding device blocks:" status bar if discard is actually implemented in the IO manager and in the kernel.  Otherwise, there is no correct test result that will work in all environments.  I'd prefer not to print an error message for the majority of devices that do not support discard, since this is confusing.
> 
> Signed-off-by: Andreas Dilger <adilger@dilger.ca>
> 
> 

Hi Andreas,

thanks for the patch, it looks good, I have almost the same one in my tree
:). I was wondering if we have any way to tell if the device support discard
or not from the user-space, I did not found any reliable way (other than
actually call BLKDISCARD and see).

Thanks!

Re: [PATCH]: e2fsprogs m_* self tests no longer pass

[Andreas Dilger]

On 2011-03-16, at 2:14 AM, Lukas Czerner wrote:
> On Tue, 15 Mar 2011, Andreas Dilger wrote:
>> Only display the "Discarding device blocks:" status bar if discard is actually implemented in the IO manager and in the kernel.  Otherwise, there is no correct test result that will work in all environments.  I'd prefer not to print an error message for the majority of devices that do not support discard, since this is confusing.
>> 
>> Signed-off-by: Andreas Dilger <adilger@dilger.ca>
> 
> Hi Andreas,
> 
> thanks for the patch, it looks good, I have almost the same one in my tree
> :). I was wondering if we have any way to tell if the device support discard
> or not from the user-space, I did not found any reliable way (other than
> actually call BLKDISCARD and see).

I was originally going to add a CHANNEL_FLAGS_DISCARD flag to the iomanager, but just setting this if BLKDISCARD is defined does not determine if the kernel or disk device has support for TRIM/UNMAP.  Since the only way to check this AFAICS is to call BLKDISCARD I changed the code as seen in the patch.

It wouldn't be terrible to set or clear the CHANNEL_FLAGS_DISCARD flag for a device depending on whether io_channel_discard() succeeds, but I suspect there will be few cases where discard is called multiple times on the same device.

Cheers, Andreas
--
Andreas Dilger 
Principal Engineer
Whamcloud, Inc.

Re: [PATCH]: e2fsprogs m_* self tests no longer pass

[Ted Ts'o]

On Tue, Mar 15, 2011 at 01:50:48PM -0600, Andreas Dilger wrote:
> Only display the "Discarding device blocks:" status bar if discard
> is actually implemented in the IO manager and in the kernel.
> Otherwise, there is no correct test result that will work in all
> environments.  I'd prefer not to print an error message for the
> majority of devices that do not support discard, since this is
> confusing.

I believe this should be fixed already by commit aa07cb79b0, which is
in the e2fsprogs master and next branches.  Can you confirm?

	  	     	  	     	     - Ted

Re: [PATCH]: e2fsprogs m_* self tests no longer pass

[Andreas Dilger]

On 2011-03-19, at 9:29 PM, Ted Ts'o wrote:
> On Tue, Mar 15, 2011 at 01:50:48PM -0600, Andreas Dilger wrote:
>> Only display the "Discarding device blocks:" status bar if discard
>> is actually implemented in the IO manager and in the kernel.
>> Otherwise, there is no correct test result that will work in all
>> environments.  I'd prefer not to print an error message for the
>> majority of devices that do not support discard, since this is
>> confusing.
> 
> I believe this should be fixed already by commit aa07cb79b0, which is
> in the e2fsprogs master and next branches.  Can you confirm?

It looks like it will do the same thing, though I don't know what the semantics are for doing a 0-length discard.  I'm reluctant to do an operation like this that may not have well-defined semantics (e.g. return EINVAL for this case).

My patch only does the requested discard, and checks the return of the first call.  It still makes sense to print an error for anything other than EXT2_ET_UNIMPLEMENTED instead of silently hiding the error.

Cheers, Andreas
--
Andreas Dilger 
Principal Engineer
Whamcloud, Inc.