[PATCH] raid0: Set discard_granularity to correct value after reshape.

[PATCH] raid0: Set discard_granularity to correct value after reshape.

[Pawel Baldysiak]

In case of reshape of raid0 through raid4 a value of discard_granularity
will be set to stripe size. MD driver should re-set this value to correct
one when migration will be finished. Otherwise array will be left with
wrong value and discard operations will not work properly.

Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
Cc: Shaohua Li <shli@kernel.org>
---
 drivers/md/raid0.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index c4d420b..807ca3a 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -266,6 +266,8 @@ static int create_strip_zones(struct mddev *mddev, struct r0conf **private_conf)
 	}
 	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
 	mddev->queue->backing_dev_info.congested_data = mddev;
+	mddev->queue->limits.discard_granularity =
+			queue_logical_block_size(mddev->queue);
 
 	/*
 	 * now since we have the hard sector sizes, we can make sure

Re: [PATCH] raid0: Set discard_granularity to correct value after reshape.

[NeilBrown]

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

On Wed, 30 Oct 2013 13:20:22 +0100 Pawel Baldysiak
<pawel.baldysiak@intel.com> wrote:

> In case of reshape of raid0 through raid4 a value of discard_granularity
> will be set to stripe size. MD driver should re-set this value to correct
> one when migration will be finished. Otherwise array will be left with
> wrong value and discard operations will not work properly.
> 
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> Cc: Shaohua Li <shli@kernel.org>
> ---
>  drivers/md/raid0.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index c4d420b..807ca3a 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -266,6 +266,8 @@ static int create_strip_zones(struct mddev *mddev, struct r0conf **private_conf)
>  	}
>  	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
>  	mddev->queue->backing_dev_info.congested_data = mddev;
> +	mddev->queue->limits.discard_granularity =
> +			queue_logical_block_size(mddev->queue);
>  
>  	/*
>  	 * now since we have the hard sector sizes, we can make sure

Thanks, but this doesn't seem like the right sort of fix.  It is to specific
to the symptom rather than trying to address the underlying problem.

Maybe something like this?  Can you review and test?

Thanks,
NeilBrown


diff --git a/drivers/md/md.c b/drivers/md/md.c
index 628cd529343f..740b6340f980 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->in_sync = 1;
 		del_timer_sync(&mddev->safemode_timer);
 	}
+	blk_set_stacking_limit(&mddev->queue->limits);
 	pers->run(mddev);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
 	mddev_resume(mddev);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH] raid0: Set discard_granularity to correct value after reshape.

[Baldysiak, Pawel]

On Thursday, October 31, 2013 1:16 AM NeilBrown <neilb@suse.de> wrote:
> On Wed, 30 Oct 2013 13:20:22 +0100 Pawel Baldysiak
> <pawel.baldysiak@intel.com> wrote:
> 
> > In case of reshape of raid0 through raid4 a value of
> > discard_granularity will be set to stripe size. MD driver should
> > re-set this value to correct one when migration will be finished.
> > Otherwise array will be left with wrong value and discard operations will not
> work properly.
> >
> > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > Cc: Shaohua Li <shli@kernel.org>
> > ---
> >  drivers/md/raid0.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c index
> > c4d420b..807ca3a 100644
> > --- a/drivers/md/raid0.c
> > +++ b/drivers/md/raid0.c
> > @@ -266,6 +266,8 @@ static int create_strip_zones(struct mddev *mddev,
> struct r0conf **private_conf)
> >  	}
> >  	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
> >  	mddev->queue->backing_dev_info.congested_data = mddev;
> > +	mddev->queue->limits.discard_granularity =
> > +			queue_logical_block_size(mddev->queue);
> >
> >  	/*
> >  	 * now since we have the hard sector sizes, we can make sure
> 
> Thanks, but this doesn't seem like the right sort of fix.  It is to specific to the
> symptom rather than trying to address the underlying problem.
> 
> Maybe something like this?  Can you review and test?
> 
> Thanks,
> NeilBrown
> 
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c index
> 628cd529343f..740b6340f980 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char
> *buf, size_t len)
>  		mddev->in_sync = 1;
>  		del_timer_sync(&mddev->safemode_timer);
>  	}
> +	blk_set_stacking_limit(&mddev->queue->limits);
>  	pers->run(mddev);
>  	set_bit(MD_CHANGE_DEVS, &mddev->flags);
>  	mddev_resume(mddev);
Hi Neil,
I have tested Yours patch, and everything works well.
TRIM operation are made correctly.
Could You apply it to upstream? 

BTW. Correct name of this function is blk_set_stacking_limits();

Thanks,
Paweł Baldysiak

Re: [PATCH] raid0: Set discard_granularity to correct value after reshape.

[NeilBrown]

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

On Tue, 5 Nov 2013 14:25:19 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@intel.com> wrote:

> On Thursday, October 31, 2013 1:16 AM NeilBrown <neilb@suse.de> wrote:
> > On Wed, 30 Oct 2013 13:20:22 +0100 Pawel Baldysiak
> > <pawel.baldysiak@intel.com> wrote:
> > 
> > > In case of reshape of raid0 through raid4 a value of
> > > discard_granularity will be set to stripe size. MD driver should
> > > re-set this value to correct one when migration will be finished.
> > > Otherwise array will be left with wrong value and discard operations will not
> > work properly.
> > >
> > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > > Cc: Shaohua Li <shli@kernel.org>
> > > ---
> > >  drivers/md/raid0.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c index
> > > c4d420b..807ca3a 100644
> > > --- a/drivers/md/raid0.c
> > > +++ b/drivers/md/raid0.c
> > > @@ -266,6 +266,8 @@ static int create_strip_zones(struct mddev *mddev,
> > struct r0conf **private_conf)
> > >  	}
> > >  	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
> > >  	mddev->queue->backing_dev_info.congested_data = mddev;
> > > +	mddev->queue->limits.discard_granularity =
> > > +			queue_logical_block_size(mddev->queue);
> > >
> > >  	/*
> > >  	 * now since we have the hard sector sizes, we can make sure
> > 
> > Thanks, but this doesn't seem like the right sort of fix.  It is to specific to the
> > symptom rather than trying to address the underlying problem.
> > 
> > Maybe something like this?  Can you review and test?
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > diff --git a/drivers/md/md.c b/drivers/md/md.c index
> > 628cd529343f..740b6340f980 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char
> > *buf, size_t len)
> >  		mddev->in_sync = 1;
> >  		del_timer_sync(&mddev->safemode_timer);
> >  	}
> > +	blk_set_stacking_limit(&mddev->queue->limits);
> >  	pers->run(mddev);
> >  	set_bit(MD_CHANGE_DEVS, &mddev->flags);
> >  	mddev_resume(mddev);
> Hi Neil,
> I have tested Yours patch, and everything works well.
> TRIM operation are made correctly.
> Could You apply it to upstream? 
> 
> BTW. Correct name of this function is blk_set_stacking_limits();
> 
> Thanks,
> Paweł Baldysiak

Thanks for testing.  I've queue the following up for submission soonish.

NeilBrown

From 3d2b31bb23ccb8170fafdf358098bb3cb0836080 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Thu, 7 Nov 2013 13:36:36 +1100
Subject: [PATCH] md: fix calculation of stacking limits on level change.

The various ->run routines of md personalities assume that the 'queue'
has been initialised by the blk_set_stacking_limits() call in
md_alloc().

However when the level is changed (by level_store()) the ->run routine
for the new level is called for an array which has already had the
stacking limits modified.  This can result in incorrect final
settings.

So call blk_set_stacking_limits() before ->run in level_store().

A specific consequence of this bug is that it causes
discard_granularity to be set incorrectly when reshaping a RAID4 to a
RAID0.

This is suitable for any -stable kernel since 3.3 in which
blk_set_stacking_limits() was introduced.

Cc: stable@vger.kernel.org (3.3+)
Reported-by: "Baldysiak, Pawel" <pawel.baldysiak@intel.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 628cd529343f..b3a75afee560 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->in_sync = 1;
 		del_timer_sync(&mddev->safemode_timer);
 	}
+	blk_set_stacking_limits(&mddev->queue->limits);
 	pers->run(mddev);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
 	mddev_resume(mddev);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]