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 <[email protected]>
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 <[email protected]>
>
>
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!
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 <[email protected]>
>
> 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.
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
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.