[PATCH] dm-mirror: do not degrade the mirror on discard error

[PATCH] dm-mirror: do not degrade the mirror on discard error

[Mikulas Patocka]

It may be possible that a device claims discard support but it rejects
discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
filesystem driven by the ext4 driver. It may also happen if the underlying
devices are moved from one disk on another.

If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
not degrade the array.

This patch fixes failed test shell/lvconvert-repair-transient.sh in the
lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
and it is being driven by the ext4 driver.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Index: linux-2.6/drivers/md/dm-raid1.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-raid1.c
+++ linux-2.6/drivers/md/dm-raid1.c
@@ -604,6 +604,15 @@ static void write_callback(unsigned long
 		return;
 	}
 
+	/*
+	 * If the bio is discard, return an error, but do not
+	 * degrade the array.
+	 */
+	if (bio->bi_rw & REQ_DISCARD) {
+		bio_endio(bio, -EOPNOTSUPP);
+		return;
+	}
+
 	for (i = 0; i < ms->nr_mirrors; i++)
 		if (test_bit(i, &error))
 			fail_mirror(ms->mirror + i, DM_RAID1_WRITE_ERROR);

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[Mikulas Patocka]

On Thu, 12 Feb 2015, Mikulas Patocka wrote:

> It may be possible that a device claims discard support but it rejects
> discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
> filesystem driven by the ext4 driver. It may also happen if the underlying
> devices are moved from one disk on another.
> 
> If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
> not degrade the array.
> 
> This patch fixes failed test shell/lvconvert-repair-transient.sh in the
> lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
> and it is being driven by the ext4 driver.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

I forgot to add

Cc: stable@kernel.org



BTW. that ext4 behavior is specific to RHEL 7. On upstream kernel, the 
ext4 driver can punch a hole in the ext2 or ext3 filesystem, but there are 
corruptions when doing it - see bug 1176840.

Mikulas

> Index: linux-2.6/drivers/md/dm-raid1.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-raid1.c
> +++ linux-2.6/drivers/md/dm-raid1.c
> @@ -604,6 +604,15 @@ static void write_callback(unsigned long
>  		return;
>  	}
>  
> +	/*
> +	 * If the bio is discard, return an error, but do not
> +	 * degrade the array.
> +	 */
> +	if (bio->bi_rw & REQ_DISCARD) {
> +		bio_endio(bio, -EOPNOTSUPP);
> +		return;
> +	}
> +
>  	for (i = 0; i < ms->nr_mirrors; i++)
>  		if (test_bit(i, &error))
>  			fail_mirror(ms->mirror + i, DM_RAID1_WRITE_ERROR);
> 

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[James Bottomley]

On Thu, 2015-02-12 at 10:09 -0500, Mikulas Patocka wrote:
> It may be possible that a device claims discard support but it rejects
> discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
> filesystem driven by the ext4 driver. It may also happen if the underlying
> devices are moved from one disk on another.
> 
> If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
> not degrade the array.
> 
> This patch fixes failed test shell/lvconvert-repair-transient.sh in the
> lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
> and it is being driven by the ext4 driver.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Index: linux-2.6/drivers/md/dm-raid1.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-raid1.c
> +++ linux-2.6/drivers/md/dm-raid1.c
> @@ -604,6 +604,15 @@ static void write_callback(unsigned long
>  		return;
>  	}
>  
> +	/*
> +	 * If the bio is discard, return an error, but do not
> +	 * degrade the array.
> +	 */
> +	if (bio->bi_rw & REQ_DISCARD) {
> +		bio_endio(bio, -EOPNOTSUPP);

I think the error gets ignored, so this is probably harmless, but
shouldn't you propagate the actual error here?  discard is advisory and
can fail for a variety of reasons (alignment being chief) for which
EOPNOTSUPP is inappropriate.

James

Re: dm-mirror: do not degrade the mirror on discard error

[Mike Snitzer]

On Sat, Feb 14 2015 at  9:39pm -0500,
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Thu, 2015-02-12 at 10:09 -0500, Mikulas Patocka wrote:
> > It may be possible that a device claims discard support but it rejects
> > discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
> > filesystem driven by the ext4 driver. It may also happen if the underlying
> > devices are moved from one disk on another.
> > 
> > If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
> > not degrade the array.
> > 
> > This patch fixes failed test shell/lvconvert-repair-transient.sh in the
> > lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
> > and it is being driven by the ext4 driver.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Index: linux-2.6/drivers/md/dm-raid1.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-raid1.c
> > +++ linux-2.6/drivers/md/dm-raid1.c
> > @@ -604,6 +604,15 @@ static void write_callback(unsigned long
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * If the bio is discard, return an error, but do not
> > +	 * degrade the array.
> > +	 */
> > +	if (bio->bi_rw & REQ_DISCARD) {
> > +		bio_endio(bio, -EOPNOTSUPP);
> 
> I think the error gets ignored, so this is probably harmless, but
> shouldn't you propagate the actual error here?  discard is advisory and
> can fail for a variety of reasons (alignment being chief) for which
> EOPNOTSUPP is inappropriate.

In general you're right.  But this particular dm-mirror code doesn't
concern itself with specific error codes.  See dm-io.c:dec_count(), any
error that occurs sets an error bit that corresponds to the raid member
that experienced the failure.  And write_callback() will just check to
see if any errors occured across all members (by checking if each raid
members' bit is set in the 'unsigned long error' passed to
write_callback).

This is the first I've dug into this aspect of the old dm-mirror code
(thankfully we have the dm-raid target that wraps MD now!).. I'm not
liking that error codes are getting dropped on the floor in dm-io.
But I don't have a strong interest in fixing this just for dm-mirror
given the code is becoming increasingly niche (really only offers
clustered mirroring now).

BUT dm-io is used by other targets, so this is could become a bigger
issue if specific error codes matter to targets issuing io using dm-io.
So far it hasn't been an issue.  But something I'll keep in mind.

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[Mikulas Patocka]

On Sat, 14 Feb 2015, James Bottomley wrote:

> On Thu, 2015-02-12 at 10:09 -0500, Mikulas Patocka wrote:
> > It may be possible that a device claims discard support but it rejects
> > discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
> > filesystem driven by the ext4 driver. It may also happen if the underlying
> > devices are moved from one disk on another.
> > 
> > If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
> > not degrade the array.
> > 
> > This patch fixes failed test shell/lvconvert-repair-transient.sh in the
> > lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
> > and it is being driven by the ext4 driver.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Index: linux-2.6/drivers/md/dm-raid1.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/dm-raid1.c
> > +++ linux-2.6/drivers/md/dm-raid1.c
> > @@ -604,6 +604,15 @@ static void write_callback(unsigned long
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * If the bio is discard, return an error, but do not
> > +	 * degrade the array.
> > +	 */
> > +	if (bio->bi_rw & REQ_DISCARD) {
> > +		bio_endio(bio, -EOPNOTSUPP);
> 
> I think the error gets ignored, so this is probably harmless, but
> shouldn't you propagate the actual error here?  discard is advisory and
> can fail for a variety of reasons (alignment being chief) for which
> EOPNOTSUPP is inappropriate.
> 
> James

dm-io doesn't pass the error code to the callback, it only returns the 
bitmask of failed devices. So, it is impossible to find the real error 
code.

Mikulas

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[James Bottomley]

On Mon, 2015-02-16 at 07:44 -0500, Mikulas Patocka wrote:
> 
> On Sat, 14 Feb 2015, James Bottomley wrote:
> 
> > On Thu, 2015-02-12 at 10:09 -0500, Mikulas Patocka wrote:
> > > It may be possible that a device claims discard support but it rejects
> > > discards with -EOPNOTSUPP. It happens when using loopback on ext2/ext3
> > > filesystem driven by the ext4 driver. It may also happen if the underlying
> > > devices are moved from one disk on another.
> > > 
> > > If discard error happens, we reject the bio with -EOPNOTSUPP, but we do
> > > not degrade the array.
> > > 
> > > This patch fixes failed test shell/lvconvert-repair-transient.sh in the
> > > lvm2 testsuite if the testsuite is extracted on an ext2 or ext3 filesystem
> > > and it is being driven by the ext4 driver.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > Index: linux-2.6/drivers/md/dm-raid1.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/md/dm-raid1.c
> > > +++ linux-2.6/drivers/md/dm-raid1.c
> > > @@ -604,6 +604,15 @@ static void write_callback(unsigned long
> > >  		return;
> > >  	}
> > >  
> > > +	/*
> > > +	 * If the bio is discard, return an error, but do not
> > > +	 * degrade the array.
> > > +	 */
> > > +	if (bio->bi_rw & REQ_DISCARD) {
> > > +		bio_endio(bio, -EOPNOTSUPP);
> > 
> > I think the error gets ignored, so this is probably harmless, but
> > shouldn't you propagate the actual error here?  discard is advisory and
> > can fail for a variety of reasons (alignment being chief) for which
> > EOPNOTSUPP is inappropriate.
> > 
> > James
> 
> dm-io doesn't pass the error code to the callback, it only returns the 
> bitmask of failed devices. So, it is impossible to find the real error 
> code.

I already said this in the first sentence of the last paragraph of my
email.  The point isn't what it does today it's what might happen
tomorrow and the principle of least surprise.  One day, someone might
propagate the error.  When that happens, they'll be surprised to find
every discard failure reported as -ENOTSUPP and it will cost someone
time and effort to investigate and fix.  If you just propagate the error
today, you save all that work in the future.

James

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[Mikulas Patocka]

On Mon, 16 Feb 2015, James Bottomley wrote:

> I already said this in the first sentence of the last paragraph of my
> email.  The point isn't what it does today it's what might happen
> tomorrow and the principle of least surprise.  One day, someone might
> propagate the error.  When that happens, they'll be surprised to find
> every discard failure reported as -ENOTSUPP and it will cost someone
> time and effort to investigate and fix.  If you just propagate the error
> today, you save all that work in the future.
> 
> James

The question is if this case is so important that it justifies dm-io 
change.

The SSD may ignore discards and report them as sucesfully completed, so no 
one should depend on the return code anyway. The error code may be used as 
a hint that it is futile to send more discards in the future, but relying 
on the return code is already not correct.

Mikulas

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[James Bottomley]

On Tue, 2015-02-17 at 14:59 -0500, Mikulas Patocka wrote:
> 
> On Mon, 16 Feb 2015, James Bottomley wrote:
> 
> > I already said this in the first sentence of the last paragraph of my
> > email.  The point isn't what it does today it's what might happen
> > tomorrow and the principle of least surprise.  One day, someone might
> > propagate the error.  When that happens, they'll be surprised to find
> > every discard failure reported as -ENOTSUPP and it will cost someone
> > time and effort to investigate and fix.  If you just propagate the error
> > today, you save all that work in the future.
> > 
> > James
> 
> The question is if this case is so important that it justifies dm-io 
> change.

I'm not sure I follow.  Are you saying no-one would ever want to
propagate the error?  I think that would be short sighted.

> The SSD may ignore discards and report them as sucesfully completed, so no 
> one should depend on the return code anyway. The error code may be used as 
> a hint that it is futile to send more discards in the future, but relying 
> on the return code is already not correct.

That's not a good way of interpreting the standards.  For instance unmap
has two types of error: permanent and transient.  Permanent means the
device would never be able to process the unmap and you should move on.
Transient means the device may be able to process the unmap and you
might like to repeat it.  Mostly the retries will be handled by SCSI but
not always.

That the discard issuer doesn't care is also not a given.  In the low
end SSD case you cite above, they probably don't.  However, if it's a
cloud environment charging per megabyte per day for provisioned
capacity, they probably do care.

The point here is that since you have the ability to do the right thing
(you have the error code the lower layer sent), just do it.  It will
save a lot of pain later on.  Doing the wrong thing and trying to
justify it post facto based on how you see the future evolving is
inevitably the wrong course of action because we're not very good at
predictions.

James

Re: [PATCH] dm-mirror: do not degrade the mirror on discard error

[Mikulas Patocka]

On Wed, 18 Feb 2015, James Bottomley wrote:

> On Tue, 2015-02-17 at 14:59 -0500, Mikulas Patocka wrote:
> > 
> > On Mon, 16 Feb 2015, James Bottomley wrote:
> > 
> > > I already said this in the first sentence of the last paragraph of my
> > > email.  The point isn't what it does today it's what might happen
> > > tomorrow and the principle of least surprise.  One day, someone might
> > > propagate the error.  When that happens, they'll be surprised to find
> > > every discard failure reported as -ENOTSUPP and it will cost someone
> > > time and effort to investigate and fix.  If you just propagate the error
> > > today, you save all that work in the future.
> > > 
> > > James
> > 
> > The question is if this case is so important that it justifies dm-io 
> > change.
> 
> I'm not sure I follow.  Are you saying no-one would ever want to
> propagate the error?  I think that would be short sighted.

The SATA device may report success on the trim command and may not trim 
any data. I know this is stupid, but the standard allows the device to do 
that and the devices are doing that. See this thread 
http://www.spinics.net/lists/linux-scsi/msg80297.html

Consequently, if some filesystem or other application contains the logic 
"if trim succeeded, do something", it is broken, because the SSD may 
ignore the command and report success.

> > The SSD may ignore discards and report them as sucesfully completed, so no 
> > one should depend on the return code anyway. The error code may be used as 
> > a hint that it is futile to send more discards in the future, but relying 
> > on the return code is already not correct.
> 
> That's not a good way of interpreting the standards.

It doesn't matter how do you interpret the standard. It does matter how do 
SSD vendors interpret it. And they interpret it in such a way that it is 
OK to report success and not trim any data. I know it doesn't make much 
sense to standardize the flag "Return Zero After Trim" and then specify 
that the device (despite having RZAT set) may ignore the trim command and 
not return zeroes on the trimmed data. But it's the way it is.

Mikulas


> For instance unmap has two types of error: permanent and transient.  
> Permanent means the device would never be able to process the unmap and 
> you should move on. Transient means the device may be able to process 
> the unmap and you might like to repeat it.  Mostly the retries will be 
> handled by SCSI but not always.
> 
> That the discard issuer doesn't care is also not a given.  In the low
> end SSD case you cite above, they probably don't.  However, if it's a
> cloud environment charging per megabyte per day for provisioned
> capacity, they probably do care.
> 
> The point here is that since you have the ability to do the right thing
> (you have the error code the lower layer sent), just do it.  It will
> save a lot of pain later on.  Doing the wrong thing and trying to
> justify it post facto based on how you see the future evolving is
> inevitably the wrong course of action because we're not very good at
> predictions.
> 
> James

Re: dm-mirror: do not degrade the mirror on discard error

[Mike Snitzer]

On Wed, Feb 18 2015 at 11:29am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 18 Feb 2015, James Bottomley wrote:
> 
> > On Tue, 2015-02-17 at 14:59 -0500, Mikulas Patocka wrote:
> > > 
> > > On Mon, 16 Feb 2015, James Bottomley wrote:
> > > 
> > > > I already said this in the first sentence of the last paragraph of my
> > > > email.  The point isn't what it does today it's what might happen
> > > > tomorrow and the principle of least surprise.  One day, someone might
> > > > propagate the error.  When that happens, they'll be surprised to find
> > > > every discard failure reported as -ENOTSUPP and it will cost someone
> > > > time and effort to investigate and fix.  If you just propagate the error
> > > > today, you save all that work in the future.
> > > > 
> > > > James
> > > 
> > > The question is if this case is so important that it justifies dm-io 
> > > change.
> > 
> > I'm not sure I follow.  Are you saying no-one would ever want to
> > propagate the error?  I think that would be short sighted.
> 
> The SATA device may report success on the trim command and may not trim 
> any data. I know this is stupid, but the standard allows the device to do 
> that and the devices are doing that. See this thread 
> http://www.spinics.net/lists/linux-scsi/msg80297.html
> 
> Consequently, if some filesystem or other application contains the logic 
> "if trim succeeded, do something", it is broken, because the SSD may 
> ignore the command and report success.
> 
> > > The SSD may ignore discards and report them as sucesfully completed, so no 
> > > one should depend on the return code anyway. The error code may be used as 
> > > a hint that it is futile to send more discards in the future, but relying 
> > > on the return code is already not correct.
> > 
> > That's not a good way of interpreting the standards.
> 
> It doesn't matter how do you interpret the standard. It does matter how do 
> SSD vendors interpret it. And they interpret it in such a way that it is 
> OK to report success and not trim any data. I know it doesn't make much 
> sense to standardize the flag "Return Zero After Trim" and then specify 
> that the device (despite having RZAT set) may ignore the trim command and 
> not return zeroes on the trimmed data. But it's the way it is.

Let's please set this thread to one side.  I already offered my thoughts
in this thread, see: 
https://www.redhat.com/archives/dm-devel/2015-February/msg00076.html

I'm not interested in wiring up the actual error return for dm-mirror's
benefit.  If/when other dm-io consumers have a need to differentiate
between error codes we'll fix dm-io as needed.

Re: dm-mirror: do not degrade the mirror on discard error

[James Bottomley]

On Wed, 2015-02-18 at 12:10 -0500, Mike Snitzer wrote:
> On Wed, Feb 18 2015 at 11:29am -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Wed, 18 Feb 2015, James Bottomley wrote:
> > 
> > > On Tue, 2015-02-17 at 14:59 -0500, Mikulas Patocka wrote:
> > > > 
> > > > On Mon, 16 Feb 2015, James Bottomley wrote:
> > > > 
> > > > > I already said this in the first sentence of the last paragraph of my
> > > > > email.  The point isn't what it does today it's what might happen
> > > > > tomorrow and the principle of least surprise.  One day, someone might
> > > > > propagate the error.  When that happens, they'll be surprised to find
> > > > > every discard failure reported as -ENOTSUPP and it will cost someone
> > > > > time and effort to investigate and fix.  If you just propagate the error
> > > > > today, you save all that work in the future.
> > > > > 
> > > > > James
> > > > 
> > > > The question is if this case is so important that it justifies dm-io 
> > > > change.
> > > 
> > > I'm not sure I follow.  Are you saying no-one would ever want to
> > > propagate the error?  I think that would be short sighted.
> > 
> > The SATA device may report success on the trim command and may not trim 
> > any data. I know this is stupid, but the standard allows the device to do 
> > that and the devices are doing that. See this thread 
> > http://www.spinics.net/lists/linux-scsi/msg80297.html
> > 
> > Consequently, if some filesystem or other application contains the logic 
> > "if trim succeeded, do something", it is broken, because the SSD may 
> > ignore the command and report success.
> > 
> > > > The SSD may ignore discards and report them as sucesfully completed, so no 
> > > > one should depend on the return code anyway. The error code may be used as 
> > > > a hint that it is futile to send more discards in the future, but relying 
> > > > on the return code is already not correct.
> > > 
> > > That's not a good way of interpreting the standards.
> > 
> > It doesn't matter how do you interpret the standard. It does matter how do 
> > SSD vendors interpret it. And they interpret it in such a way that it is 
> > OK to report success and not trim any data. I know it doesn't make much 
> > sense to standardize the flag "Return Zero After Trim" and then specify 
> > that the device (despite having RZAT set) may ignore the trim command and 
> > not return zeroes on the trimmed data. But it's the way it is.
> 
> Let's please set this thread to one side.  I already offered my thoughts
> in this thread, see: 
> https://www.redhat.com/archives/dm-devel/2015-February/msg00076.html
> 
> I'm not interested in wiring up the actual error return for dm-mirror's
> benefit.  If/when other dm-io consumers have a need to differentiate
> between error codes we'll fix dm-io as needed.

Yep, concur ... the point I've been obviously failing to make is that
all the world is not an SSD.

James

Re: dm-mirror: do not degrade the mirror on discard error

[Mike Snitzer]

On Wed, Feb 18 2015 at 12:54pm -0500,
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2015-02-18 at 12:10 -0500, Mike Snitzer wrote:
> > 
> > Let's please set this thread to one side.  I already offered my thoughts
> > in this thread, see: 
> > https://www.redhat.com/archives/dm-devel/2015-February/msg00076.html
> > 
> > I'm not interested in wiring up the actual error return for dm-mirror's
> > benefit.  If/when other dm-io consumers have a need to differentiate
> > between error codes we'll fix dm-io as needed.
> 
> Yep, concur ... the point I've been obviously failing to make is that
> all the world is not an SSD.

I agree, and am aware.  The layers that have needed to worry about
properly differentiating between these higher level returns from SCSI do
(e.g. dm-mpath).  But it is pretty rare for bio-based DM targets to
differentiate between error codes.  Not saying that is right, just that
is how things stand.  Could be there is room for improvement in places.