[dm-devel] 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 at 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 at 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.