* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-27 20:19 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Boris Ostrovsky, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <20170426073720.okv33ly2ldepilti-aUbyMND+kyB2Oba8jWPag5QscXo+jHNAQQ4Iyu8u01E@public.gmane.org>
On 26/04/17 01:37 AM, Roger Pau Monné wrote:
> On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote:
>> Straightforward conversion to the new helper, except due to the lack
>> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in
>> certain cases in the future.
>>
>> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
>> Cc: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> Cc: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> Cc: "Roger Pau Monné" <roger.pau-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
>> ---
>> drivers/block/xen-blkfront.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index 3945963..ed62175 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
>> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
>>
>> if (setup.need_copy) {
>> - setup.bvec_off = sg->offset;
>> - setup.bvec_data = kmap_atomic(sg_page(sg));
>> + setup.bvec_off = 0;
>> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC |
>> + SG_MAP_MUST_NOT_FAIL);
>
> I assume that sg_map already adds sg->offset to the address?
Correct.
> Also wondering whether we can get rid of bvec_off and just increment bvec_data,
> adding Julien who IIRC added this code.
bvec_off is used to keep track of the offset within the current mapping
so it's not a great idea given that you'd want to kunmap_atomic the
original address and not something with an offset. It would be nice if
this could be converted to use the sg_miter interface but that's a much
more invasive change that would require someone who knows this code and
can properly test it. I'd be very grateful if someone actually took that on.
Logan
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Jason Gunthorpe @ 2017-04-27 20:53 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm-y27Ovi1pjclAfugRpC6u6w,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <df6586e2-7d45-6b0b-facb-4dea882df06e-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
On Thu, Apr 27, 2017 at 02:19:24PM -0600, Logan Gunthorpe wrote:
>
>
> On 26/04/17 01:37 AM, Roger Pau Monné wrote:
> > On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote:
> >> Straightforward conversion to the new helper, except due to the lack
> >> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in
> >> certain cases in the future.
> >>
> >> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
> >> Cc: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >> Cc: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >> Cc: "Roger Pau Monné" <roger.pau-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
> >> drivers/block/xen-blkfront.c | 20 +++++++++++---------
> >> 1 file changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >> index 3945963..ed62175 100644
> >> +++ b/drivers/block/xen-blkfront.c
> >> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
> >> BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >>
> >> if (setup.need_copy) {
> >> - setup.bvec_off = sg->offset;
> >> - setup.bvec_data = kmap_atomic(sg_page(sg));
> >> + setup.bvec_off = 0;
> >> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC |
> >> + SG_MAP_MUST_NOT_FAIL);
> >
> > I assume that sg_map already adds sg->offset to the address?
>
> Correct.
>
> > Also wondering whether we can get rid of bvec_off and just increment bvec_data,
> > adding Julien who IIRC added this code.
>
> bvec_off is used to keep track of the offset within the current mapping
> so it's not a great idea given that you'd want to kunmap_atomic the
> original address and not something with an offset. It would be nice if
> this could be converted to use the sg_miter interface but that's a much
> more invasive change that would require someone who knows this code and
> can properly test it. I'd be very grateful if someone actually took that on.
blkfront is one of the drivers I looked at, and it appears to only be
memcpying with the bvec_data pointer, so I wonder why it does not use
sg_copy_X_buffer instead..
Jason
^ permalink raw reply
* Re: [MD PATCH v2 1/1] Use a new variable to count flighting sync requests
From: Shaohua Li @ 2017-04-27 20:58 UTC (permalink / raw)
To: Xiao Ni; +Cc: linux-raid, colyli, ncroxon
In-Reply-To: <1493281729-5366-1-git-send-email-xni@redhat.com>
On Thu, Apr 27, 2017 at 04:28:49PM +0800, Xiao Ni wrote:
> In new barrier codes, raise_barrier waits if conf->nr_pending[idx] is not zero.
> After all the conditions are true, the resync request can go on be handled. But
> it adds conf->nr_pending[idx] again. The next resync request hit the same bucket
> idx need to wait the resync request which is submitted before. The performance
> of resync/recovery is degraded.
> So we should use a new variable to count sync requests which are in flight.
>
> I did a simple test:
> 1. Without the patch, create a raid1 with two disks. The resync speed:
> Device: rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await r_await w_await svctm %util
> sdb 0.00 0.00 166.00 0.00 10.38 0.00 128.00 0.03 0.20 0.20 0.00 0.19 3.20
> sdc 0.00 0.00 0.00 166.00 0.00 10.38 128.00 0.96 5.77 0.00 5.77 5.75 95.50
> 2. With the patch, the result is:
> sdb 2214.00 0.00 766.00 0.00 185.69 0.00 496.46 2.80 3.66 3.66 0.00 1.03 79.10
> sdc 0.00 2205.00 0.00 769.00 0.00 186.44 496.52 5.25 6.84 0.00 6.84 1.30 100.10
>
> Suggested-by: Shaohua Li <shli@kernel.org>
> Signed-off-by: Xiao Ni <xni@redhat.com>
applied, thanks!
> ---
> drivers/md/raid1.c | 5 +++--
> drivers/md/raid1.h | 1 +
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index a34f587..ff5ee53 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -869,7 +869,7 @@ static void raise_barrier(struct r1conf *conf, sector_t sector_nr)
> atomic_read(&conf->barrier[idx]) < RESYNC_DEPTH,
> conf->resync_lock);
>
> - atomic_inc(&conf->nr_pending[idx]);
> + atomic_inc(&conf->nr_sync_pending);
> spin_unlock_irq(&conf->resync_lock);
> }
>
> @@ -880,7 +880,7 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr)
> BUG_ON(atomic_read(&conf->barrier[idx]) <= 0);
>
> atomic_dec(&conf->barrier[idx]);
> - atomic_dec(&conf->nr_pending[idx]);
> + atomic_dec(&conf->nr_sync_pending);
> wake_up(&conf->wait_barrier);
> }
>
> @@ -1017,6 +1017,7 @@ static int get_unqueued_pending(struct r1conf *conf)
> {
> int idx, ret;
>
> + ret = atomic_read(&conf->nr_sync_pending);
> for (ret = 0, idx = 0; idx < BARRIER_BUCKETS_NR; idx++)
> ret += atomic_read(&conf->nr_pending[idx]) -
> atomic_read(&conf->nr_queued[idx]);
> diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> index dd22a37..1668f22 100644
> --- a/drivers/md/raid1.h
> +++ b/drivers/md/raid1.h
> @@ -84,6 +84,7 @@ struct r1conf {
> */
> wait_queue_head_t wait_barrier;
> spinlock_t resync_lock;
> + atomic_t nr_sync_pending;
> atomic_t *nr_pending;
> atomic_t *nr_waiting;
> atomic_t *nr_queued;
> --
> 2.7.4
>
^ permalink raw reply
* Re: [MD PATCH v2 1/1] Use a new variable to count flighting sync requests
From: Shaohua Li @ 2017-04-27 21:05 UTC (permalink / raw)
To: Xiao Ni; +Cc: linux-raid, colyli, ncroxon
In-Reply-To: <20170427205801.ev7hfj4vkgttlqex@kernel.org>
On Thu, Apr 27, 2017 at 01:58:01PM -0700, Shaohua Li wrote:
> On Thu, Apr 27, 2017 at 04:28:49PM +0800, Xiao Ni wrote:
> > In new barrier codes, raise_barrier waits if conf->nr_pending[idx] is not zero.
> > After all the conditions are true, the resync request can go on be handled. But
> > it adds conf->nr_pending[idx] again. The next resync request hit the same bucket
> > idx need to wait the resync request which is submitted before. The performance
> > of resync/recovery is degraded.
> > So we should use a new variable to count sync requests which are in flight.
> >
> > I did a simple test:
> > 1. Without the patch, create a raid1 with two disks. The resync speed:
> > Device: rrqm/s wrqm/s r/s w/s rMB/s wMB/s avgrq-sz avgqu-sz await r_await w_await svctm %util
> > sdb 0.00 0.00 166.00 0.00 10.38 0.00 128.00 0.03 0.20 0.20 0.00 0.19 3.20
> > sdc 0.00 0.00 0.00 166.00 0.00 10.38 128.00 0.96 5.77 0.00 5.77 5.75 95.50
> > 2. With the patch, the result is:
> > sdb 2214.00 0.00 766.00 0.00 185.69 0.00 496.46 2.80 3.66 3.66 0.00 1.03 79.10
> > sdc 0.00 2205.00 0.00 769.00 0.00 186.44 496.52 5.25 6.84 0.00 6.84 1.30 100.10
> >
> > Suggested-by: Shaohua Li <shli@kernel.org>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
>
> applied, thanks!
> > ---
> > drivers/md/raid1.c | 5 +++--
> > drivers/md/raid1.h | 1 +
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index a34f587..ff5ee53 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -869,7 +869,7 @@ static void raise_barrier(struct r1conf *conf, sector_t sector_nr)
> > atomic_read(&conf->barrier[idx]) < RESYNC_DEPTH,
> > conf->resync_lock);
> >
> > - atomic_inc(&conf->nr_pending[idx]);
> > + atomic_inc(&conf->nr_sync_pending);
> > spin_unlock_irq(&conf->resync_lock);
> > }
> >
> > @@ -880,7 +880,7 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr)
> > BUG_ON(atomic_read(&conf->barrier[idx]) <= 0);
> >
> > atomic_dec(&conf->barrier[idx]);
> > - atomic_dec(&conf->nr_pending[idx]);
> > + atomic_dec(&conf->nr_sync_pending);
> > wake_up(&conf->wait_barrier);
> > }
> >
> > @@ -1017,6 +1017,7 @@ static int get_unqueued_pending(struct r1conf *conf)
> > {
> > int idx, ret;
> >
> > + ret = atomic_read(&conf->nr_sync_pending);
> > for (ret = 0, idx = 0; idx < BARRIER_BUCKETS_NR; idx++)
actually I deleted the 'ret = 0'
> > ret += atomic_read(&conf->nr_pending[idx]) -
> > atomic_read(&conf->nr_queued[idx]);
> > diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> > index dd22a37..1668f22 100644
> > --- a/drivers/md/raid1.h
> > +++ b/drivers/md/raid1.h
> > @@ -84,6 +84,7 @@ struct r1conf {
> > */
> > wait_queue_head_t wait_barrier;
> > spinlock_t resync_lock;
> > + atomic_t nr_sync_pending;
> > atomic_t *nr_pending;
> > atomic_t *nr_waiting;
> > atomic_t *nr_queued;
> > --
> > 2.7.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-27 21:53 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm-y27Ovi1pjclAfugRpC6u6w,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <20170427205339.GB26330-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On 27/04/17 02:53 PM, Jason Gunthorpe wrote:
> blkfront is one of the drivers I looked at, and it appears to only be
> memcpying with the bvec_data pointer, so I wonder why it does not use
> sg_copy_X_buffer instead..
Yes, sort of...
But you'd potentially end up calling sg_copy_to_buffer multiple times
per page within the sg (given that gnttab_foreach_grant_in_range might
call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times).
Even calling sg_copy_to_buffer once per page seems rather inefficient as
it uses sg_miter internally.
Switching the for_each_sg to sg_miter is probably the nicer solution as
it takes care of the mapping and the offset/length accounting for you
and will have similar performance.
But, yes, if performance is not an issue, switching it to
sg_copy_to_buffer would be a less invasive change than sg_miter. Which
the same might be said about a lot of these cases.
Unfortunately, changing from kmap_atomic (which is a null operation in a
lot of cases) to sg_copy_X_buffer is a pretty big performance hit.
Logan
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Jason Gunthorpe @ 2017-04-27 22:11 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm-y27Ovi1pjclAfugRpC6u6w,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <02ba3c7b-5fab-a06c-fbbf-c3be1c0fae1b-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote:
> On 27/04/17 02:53 PM, Jason Gunthorpe wrote:
> > blkfront is one of the drivers I looked at, and it appears to only be
> > memcpying with the bvec_data pointer, so I wonder why it does not use
> > sg_copy_X_buffer instead..
>
> But you'd potentially end up calling sg_copy_to_buffer multiple times
> per page within the sg (given that gnttab_foreach_grant_in_range might
> call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times).
> Even calling sg_copy_to_buffer once per page seems rather inefficient as
> it uses sg_miter internally.
Well, that is in the current form, with more users it would make sense
to optimize for the single page case, eg by providing the existing
call, providing a faster single-page-only variant of the copy, perhaps
even one that is inlined.
> Switching the for_each_sg to sg_miter is probably the nicer solution as
> it takes care of the mapping and the offset/length accounting for you
> and will have similar performance.
sg_miter will still fail when the sg contains __iomem, however I would
expect that the sg_copy will work with iomem, by using the __iomem
memcpy variant.
So, sg_copy should always be preferred in this new world with mixed
__iomem since it is the only primitive that can transparently handle
it.
Jason
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-27 23:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm-y27Ovi1pjclAfugRpC6u6w,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <20170427221132.GA30036-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On 27/04/17 04:11 PM, Jason Gunthorpe wrote:
> On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote:
> Well, that is in the current form, with more users it would make sense
> to optimize for the single page case, eg by providing the existing
> call, providing a faster single-page-only variant of the copy, perhaps
> even one that is inlined.
Ok, does it make sense then to have an sg_copy_page_to_buffer (or some
such... I'm having trouble thinking of a sane name that isn't too long).
That just does k(un)map_atomic and memcpy? I could try that if it makes
sense to people.
>> Switching the for_each_sg to sg_miter is probably the nicer solution as
>> it takes care of the mapping and the offset/length accounting for you
>> and will have similar performance.
>
> sg_miter will still fail when the sg contains __iomem, however I would
> expect that the sg_copy will work with iomem, by using the __iomem
> memcpy variant.
Yes, that's true. Any sg_miters that ever see iomem will need to be
converted to support it. This isn't much different than the other
kmap(sg_page()) users I was converting that will also fail if they see
iomem. Though, I suspect an sg_miter user would be easier to convert to
iomem than a random kmap user.
Logan
^ permalink raw reply
* Re: Recover array after I panicked
From: Andreas Klauer @ 2017-04-27 23:12 UTC (permalink / raw)
To: Patrik Dahlström; +Cc: Brad Campbell, linux-raid
In-Reply-To: <5c3f7876-1b25-7020-5e7d-01776e7c9903@powerlamerz.org>
On Thu, Apr 27, 2017 at 09:57:20PM +0200, Patrik Dahlström wrote:
> Success! Using a 126M as data offset gave me valid data for the 5 disk
> raid and using linear device mapping I'm able to access my data again.
Nice.
> Some is probably corrupted from my previous destructive recovery
> attempts, but it seems like most of the data is still there and
> accessible. I will start backup up the most essential data now.
If nothing else happened, I'd expect it to be intact (or at least
not any more corrupt than it would be after a regular power loss).
If you encounter any files with corrupted contents, you could use
`filefrag` or `hdparm --fibmap` to determine where it is physically
located and then perhaps see if that's anywhere near your danger zone.
It could still mean you didn't choose the correct X for linear mapping.
But of course, I don't know everything that happened. :-P
> > This is only a way to get read access at the data, making the raid
> > work as a standalone again (resume reshape with lost raid metadata)
> > is another problem, to be tackled after you backed up your data ;)
>
> Now, how do we do this?
Well, there's an elegant approach that works beautifully and perfectly
and entirely risk free... and then there's a simple, stupid, quick&dirty.
Only problem is, I'm way too lazy to describe the elegant one. Shucks.
The quick and dirty method is... grow using dd from 5disk raid (overlay)
to 6disk raid (no overlay) starting from the correct offset (verify!).
That resumes your reshape in an offline-ish, hackish manner, and once
it's done you have the whole thing. That's the theory anyway.
Pseudocode:
mdadm --create /dev/md5 --assume-clean /dev/overlay/{a,b,c}
mdadm --create /dev/md6 --assume-clean /dev/{a,b,c,d} # +1 drive
losetup --offset=X /dev/loop55 /dev/md5
losetup --offset=X /dev/loop66 /dev/md6
cmp /dev/loop55 /dev/loop66 # much identical, so wow
dd bs=1M status=progress if=/dev/loop55 of=/dev/loop66
It's dangerous, okay?
I didn't tell you to do it, okay?
Don't blame me, okay?
Backup your stuff first, okay?
As this writes to your drives, you have only one shot to get it right.
You're not allowed to mount until dd is done. It's an offline operation.
If dd were to be aborted for any reason, the offset would shift to X+n.
If dd were to encounter write errors without aborting, corruption ensues.
Regards
Andreas Klauer
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Jason Gunthorpe @ 2017-04-27 23:20 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm-y27Ovi1pjclAfugRpC6u6w,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA, Juergen Gross, Julien Grall,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
In-Reply-To: <3a7c0d27-0744-4e91-b37f-3885c50455e8-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
On Thu, Apr 27, 2017 at 05:03:45PM -0600, Logan Gunthorpe wrote:
>
>
> On 27/04/17 04:11 PM, Jason Gunthorpe wrote:
> > On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote:
> > Well, that is in the current form, with more users it would make sense
> > to optimize for the single page case, eg by providing the existing
> > call, providing a faster single-page-only variant of the copy, perhaps
> > even one that is inlined.
>
> Ok, does it make sense then to have an sg_copy_page_to_buffer (or some
> such... I'm having trouble thinking of a sane name that isn't too long).
> That just does k(un)map_atomic and memcpy? I could try that if it makes
> sense to people.
It seems the most robust: test for iomem, and jump to a slow path
copy, otherwise inline the kmap and memcpy
Every place doing memcpy from sgl will need that pattern to be
correct.
> > sg_miter will still fail when the sg contains __iomem, however I would
> > expect that the sg_copy will work with iomem, by using the __iomem
> > memcpy variant.
>
> Yes, that's true. Any sg_miters that ever see iomem will need to be
> converted to support it. This isn't much different than the other
> kmap(sg_page()) users I was converting that will also fail if they see
> iomem. Though, I suspect an sg_miter user would be easier to convert to
> iomem than a random kmap user.
How? sg_miter seems like the next nightmare down this path, what is
sg_miter_next supposed to do when something hits an iomem sgl?
miter.addr is supposed to be a kernel pointer that must not be
__iomem..
Jason
^ permalink raw reply
* Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
From: Logan Gunthorpe @ 2017-04-27 23:29 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Boris Ostrovsky, linux-nvdimm, dri-devel, Stephen Bates, dm-devel,
target-devel, Christoph Hellwig, devel, James E.J. Bottomley,
linux-scsi, Matthew Wilcox, linux-rdma, Sumit Semwal,
Ross Zwisler, open-iscsi, linux-media, Juergen Gross,
Julien Grall, Konrad Rzeszutek Wilk, intel-gfx, sparmaintainer,
linux-raid, Dan Williams, megaraidlinux.pdl, Jens Axboe,
"Martin K. Petersen" <martin.p>
In-Reply-To: <20170427232022.GA30398@obsidianresearch.com>
On 27/04/17 05:20 PM, Jason Gunthorpe wrote:
> It seems the most robust: test for iomem, and jump to a slow path
> copy, otherwise inline the kmap and memcpy
>
> Every place doing memcpy from sgl will need that pattern to be
> correct.
Ok, sounds like a good place to start to me. I'll see what I can do for
a v3 of this set. Though, I probably won't send anything until after the
merge window.
>>> sg_miter will still fail when the sg contains __iomem, however I would
>>> expect that the sg_copy will work with iomem, by using the __iomem
>>> memcpy variant.
>>
>> Yes, that's true. Any sg_miters that ever see iomem will need to be
>> converted to support it. This isn't much different than the other
>> kmap(sg_page()) users I was converting that will also fail if they see
>> iomem. Though, I suspect an sg_miter user would be easier to convert to
>> iomem than a random kmap user.
>
> How? sg_miter seems like the next nightmare down this path, what is
> sg_miter_next supposed to do when something hits an iomem sgl?
My proposal is roughly included in the draft I sent upthread. We add an
sg_miter flag indicating the iteratee supports iomem and if miter finds
iomem (with the support flag set) it sets ioaddr which is __iomem. The
iteratee then just needs to null check addr and ioaddr and perform the
appropriate action.
Logan
^ permalink raw reply
* Re: [PATCH v3] md/r5cache: gracefully handle journal device errors for writeback mode
From: Shaohua Li @ 2017-04-28 0:19 UTC (permalink / raw)
To: Song Liu
Cc: linux-raid, shli, neilb, kernel-team, dan.j.williams, hch,
jes.sorensen
In-Reply-To: <20170427054302.3363710-1-songliubraving@fb.com>
On Wed, Apr 26, 2017 at 10:43:02PM -0700, Song Liu wrote:
> For the raid456 with writeback cache, when journal device failed during
> normal operation, it is still possible to persist all data, as all
> pending data is still in stripe cache. However, it is necessary to handle
> journal failure gracefully.
>
> During journal failures, this patch makes the follow changes to land data
> in cache to raid disks gracefully:
>
> 1. In raid5_remove_disk(), flush all cached stripes;
> 2. In handle_stripe(), allow stripes with data in journal (s.injournal > 0)
> to make progress;
> 3. In delay_towrite(), only process data in the cache (skip dev->towrite);
> 4. In __get_priority_stripe(), set try_loprio to true, so no stripe stuck
> in loprio_list
> 5. In r5l_do_submit_io(), submit io->split_bio first (see inline comments
> for details).
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> drivers/md/raid5-cache.c | 27 ++++++++++++++++++---------
> drivers/md/raid5.c | 37 +++++++++++++++++++++++++++++++------
> 2 files changed, 49 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index b6194e0..6b922d3 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -632,20 +632,29 @@ static void r5l_do_submit_io(struct r5l_log *log, struct r5l_io_unit *io)
> __r5l_set_io_unit_state(io, IO_UNIT_IO_START);
> spin_unlock_irqrestore(&log->io_list_lock, flags);
>
> + /*
> + * In case of journal device failures, submit_bio will get error
> + * and calls endio, then active stripes will continue write
> + * process. Therefore, it is not necessary to check Faulty bit
> + * of journal device here.
> + *
> + * However, calling r5l_log_endio for current_bio may free the
> + * io_unit. Therefore, it is necessary to check io->split_bio
> + * before submitting io->current_bio.
> + */
> + if (io->split_bio) {
> + if (io->has_flush)
> + io->split_bio->bi_opf |= REQ_PREFLUSH;
> + if (io->has_fua)
> + io->split_bio->bi_opf |= REQ_FUA;
> + submit_bio(io->split_bio);
> + }
So this could happen even there is no IO error if the bio finishes very fast.
Nice catch!
> if (io->has_flush)
> io->current_bio->bi_opf |= REQ_PREFLUSH;
> if (io->has_fua)
> io->current_bio->bi_opf |= REQ_FUA;
> submit_bio(io->current_bio);
> -
> - if (!io->split_bio)
> - return;
> -
> - if (io->has_flush)
> - io->split_bio->bi_opf |= REQ_PREFLUSH;
> - if (io->has_fua)
> - io->split_bio->bi_opf |= REQ_FUA;
> - submit_bio(io->split_bio);
> }
>
> /* deferred io_unit will be dispatched here */
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 356cd9c..2fb67c2 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -234,10 +234,12 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh,
> if (test_bit(R5_InJournal, &sh->dev[i].flags))
> injournal++;
> /*
> - * When quiesce in r5c write back, set STRIPE_HANDLE for stripes with
> - * data in journal, so they are not released to cached lists
> + * With writeback journal, stripes with data in the journal are
> + * released to cached lists. However, when journal device is
> + * failed or in quiesce, these stripes need to be handled now.
> */
> - if (conf->quiesce && r5c_is_writeback(conf->log) &&
> + if ((conf->quiesce || r5l_log_disk_error(conf)) &&
> + r5c_is_writeback(conf->log) &&
r5c_update_on_rdev_error() will switch log to write through, so we might miss
some stripes here. This brings a question. Should r5c_disable_writeback_async()
wait all stripe flushed and then switch to write through? mddev_suspend()
doesn't wait for stripe with cache, it only waits for normal IO. Waitting for
all stripes flushed and then switching to write through sounds more reasonable
to me.
> !test_bit(STRIPE_HANDLE, &sh->state) && injournal != 0) {
> if (test_bit(STRIPE_R5C_CACHING, &sh->state))
> r5c_make_stripe_write_out(sh);
> @@ -2694,6 +2696,15 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
> bdevname(rdev->bdev, b),
> mdname(mddev),
> conf->raid_disks - mddev->degraded);
> +
> + if (test_bit(Journal, &rdev->flags) && r5c_is_writeback(conf->log)) {
> + pr_warn("md/raid:%s: Journal device failed. Flushing data in the writeback cache.",
> + mdname(mddev));
> + spin_lock_irqsave(&conf->device_lock, flags);
> + r5c_flush_cache(conf, INT_MAX);
> + spin_unlock_irqrestore(&conf->device_lock, flags);
> + }
I'd suggest moving this to r5c_disable_writeback_async(). We don't do too much
in raid5_error.
> r5c_update_on_rdev_error(mddev);
> }
>
> @@ -3055,6 +3066,11 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous)
> * When LOG_CRITICAL, stripes with injournal == 0 will be sent to
> * no_space_stripes list.
> *
> + * 3. during journal failure
> + * In journal failure, we try to flush all cached data to raid disks
> + * based on data in stripe cache. The array is read-only to upper
> + * layers, so we would skip all pending writes.
> + *
> */
> static inline bool delay_towrite(struct r5conf *conf,
> struct r5dev *dev,
> @@ -3068,6 +3084,9 @@ static inline bool delay_towrite(struct r5conf *conf,
> if (test_bit(R5C_LOG_CRITICAL, &conf->cache_state) &&
> s->injournal > 0)
> return true;
> + /* case 3 above */
> + if (s->log_failed && s->injournal)
> + return true;
> return false;
> }
>
> @@ -4701,10 +4720,15 @@ static void handle_stripe(struct stripe_head *sh)
> " to_write=%d failed=%d failed_num=%d,%d\n",
> s.locked, s.uptodate, s.to_read, s.to_write, s.failed,
> s.failed_num[0], s.failed_num[1]);
> - /* check if the array has lost more than max_degraded devices and,
> + /*
> + * check if the array has lost more than max_degraded devices and,
> * if so, some requests might need to be failed.
> + *
> + * When journal device failed (log_failed), we will only process
> + * the stripe if there is data need write to raid disks
> */
> - if (s.failed > conf->max_degraded || s.log_failed) {
> + if (s.failed > conf->max_degraded ||
> + (s.log_failed && s.injournal == 0)) {
> sh->check_state = 0;
> sh->reconstruct_state = 0;
> break_stripe_batch_list(sh, 0);
> @@ -5280,7 +5304,8 @@ static struct stripe_head *__get_priority_stripe(struct r5conf *conf, int group)
> struct list_head *handle_list = NULL;
> struct r5worker_group *wg;
> bool second_try = !r5c_is_writeback(conf->log);
> - bool try_loprio = test_bit(R5C_LOG_TIGHT, &conf->cache_state);
> + bool try_loprio = test_bit(R5C_LOG_TIGHT, &conf->cache_state) ||
> + r5l_log_disk_error(conf);
>
> again:
> wg = NULL;
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* How important is SCTERC?
From: Ram Ramesh @ 2017-04-28 3:35 UTC (permalink / raw)
To: Linux Raid
Experts,
I ordered a couple of 6TB drives from goHarddrive that claimed to be NAS
suitable, but SMART shows that they do not support
scterc. I want to replace two of my 3TB drives with these in mdadm
raid6. I can get a replacement or return, but want to avoid the hassle,
if I am not seious danger.
I do run tler.sh that sets 180s timeout for drives that do not have
scterc from /etc/rc.local. Am I putting my data in serious danger? I
only have videos and tv recordings in this md which can be replaced with
significant effort (about 12TB of movies and tv shows will take some
time to copy from my DVDs/BDs)
Ramesh
^ permalink raw reply
* Re: [mdadm PATCH 4/4] Create: tell udev device is not ready when first created.
From: NeilBrown @ 2017-04-28 3:55 UTC (permalink / raw)
To: Peter Rajnoha, Jes Sorensen; +Cc: linux-raid, dm-devel
In-Reply-To: <40a5fbff-9eef-07b3-fe1b-fb9a888cfb8b@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4864 bytes --]
On Wed, Apr 26 2017, Peter Rajnoha wrote:
> On 04/20/2017 11:35 PM, NeilBrown wrote:
>> On Thu, Apr 20 2017, Jes Sorensen wrote:
> ...
>>> Second, isn't this going to be racey if you have multiple arrays
>>> running? I am wondering if we cannot find a solution that relies on a
>>> permanently installed udev rule that we enable/disable with systemctl
>>> and use the device name as an argument?
>>
>> There would only be a problematic race of an array as being created at
>> the same time that another array is being assembled. Is that at all
>> likely? They tend to happen at different parts of the usage cycle... I
>> guess we shouldn't assume though.
>>
>> I admit that blocking *all* arrays was a short cut. We need to create
>> the udev rule before the new device is first activated, and keep it in
>> existence until after the array has been started and the fd on /dev/mdXX
>> has been closed - and then a bit more because udev doesn't respond
>> instantly.
>> In some cases we don't know the name of the md device until
>> create_mddev() chooses on with find_free_devnum().
>> So if we want to block udev from handling a device, we need to put the
>> block into effect (e.g. create the rules.d file) in mddev_create() just
>> before the call to open_dev_excl().
>>
>> If we wanted an more permanent udev rule, we would need to record the
>> devices that should be ignored in the filesystem somewhere else.
>> Maybe in /run/mdadm.
>> e.g.
>>
>> KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
>>
>> Then we could have something like the following (untested) in mdadm.
>> Does that seem more suitable?
>>
>
> Yes, please, if possible, go for a permanent udev rule surely - this
> will make it much easier for other foreign tools to hook in properly if
> needed and it would also be much easier to debug.
I'm leaning towards the files-in-/run/mdadm approach too. I'll make a
proper patch.
>
> But, wouldn't it be better if we could just pass this information ("not
> initialized yet") as RUN_ARRAY md ioctl parameter? In that case the md
> driver in kernel could add the variable to the uevent it generates which
> userspace udev rules could check for easily. This way, we don't need to
> hassle with creating files in filesystem and the information would be
> directly a part of the uevent the md kernel driver generates (so
> directly accessible in udev rules too). Also, possibly adding more
> variables for other future scenarios if needed.
When would we clear the "not initialised yet" flag in the kernel, and
how? And would that be enough.
When mdadm creates an md array, at least 3 uevents get generated.
The first is generated when the md device object is created, either by
opening the /dev/mdXX file, or by writing magic to somewhere in sysfs.
The second is generated when the array is started and the content is
visible.
The third is generated when mdadm closes the file descriptor. It opens
/dev/mdXX for O_RDWR and performs ioctls on this, and then closes it.
Because udev uses inotify to watch for a 'close for a writable file
descriptor', this generates another event.
We need to ensure that none of these cause udev to run anything that
inspects the contents of the array.
Of the three, only the second one is directly under the control of the
md module, so only that one can add an environment variable.
It might be possible to avoid the last one (by not opening for write),
but I cannot see a way to avoid the first one.
I don't think that making a file appear in /run is really very different
from making a variable appear in a uevent. If the variable were
describing the event itself there would be a different, but it would be
describing the state of the device. So the only important difference is
"which is easier to work with". I think creating an deleting a file is
easier to setting and clearing a variable.
Thanks,
NeilBrown
>
> We use something similar in device-mapper already where we pass flags
> (besides other things) as a device-mapper ioctl parameter which then
> gets into the uevent as DM_COOKIE uevent variable (which in turn the
> udev rules can decode into individual flags). This method proved to be
> working well for us to solve the problem of uninitialized data area so
> we don't get into trouble with scans from udev (see also dm_ioctl
> structure in usr/include/linux/dm-ioctl.h and the dm_kobject_uevent
> function in drivers/md/dm.c for the kernel part; in udev rules the flags
> are then various DM_UDEV_* variables we check for)
>
> --
> Peter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [mdadm PATCH] Create: tell udev md device is not ready when first created.
From: NeilBrown @ 2017-04-28 5:05 UTC (permalink / raw)
To: Jes Sorensen; +Cc: Peter Rajnoha, linux-raid, dm-devel
In-Reply-To: <40a5fbff-9eef-07b3-fe1b-fb9a888cfb8b@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 9293 bytes --]
When an array is created the content is not initialized,
so it could have remnants of an old filesystem or md array
etc on it.
udev will see this and might try to activate it, which is almost
certainly not what is wanted.
So create a mechanism for mdadm to communicate with udev to tell
it that the device isn't ready. This mechanism is the existance
of a file /run/mdadm/created-mdXXX where mdXXX is the md device name.
When creating an array, mdadm will create the file.
A new udev rule file, 01-md-raid-creating.rules, will detect the
precense of thst file and set ENV{SYSTEMD_READY}="0".
This is fairly uniformly used to suppress actions based on the
contents of the device.
Signed-off-by: NeilBrown <neilb@suse.com>
---
Assemble.c | 2 +-
Build.c | 2 +-
Create.c | 9 +++++++-
Incremental.c | 4 ++--
Makefile | 4 ++--
lib.c | 29 +++++++++++++++++++++++++
mdadm.h | 4 +++-
mdopen.c | 52 ++++++++++++++++++++++++++++-----------------
udev-md-raid-creating.rules | 7 ++++++
9 files changed, 86 insertions(+), 27 deletions(-)
create mode 100644 udev-md-raid-creating.rules
diff --git a/Assemble.c b/Assemble.c
index d6beb23da9c5..a9442c8ce73b 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1478,7 +1478,7 @@ try_again:
name = strchr(name, ':')+1;
mdfd = create_mddev(mddev, name, ident->autof, trustworthy,
- chosen_name);
+ chosen_name, 0);
}
if (mdfd < 0) {
st->ss->free_super(st);
diff --git a/Build.c b/Build.c
index 11ba12f4ae7d..665d9067b8d6 100644
--- a/Build.c
+++ b/Build.c
@@ -109,7 +109,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
/* We need to create the device. It can have no name. */
map_lock(&map);
mdfd = create_mddev(mddev, NULL, c->autof, LOCAL,
- chosen_name);
+ chosen_name, 0);
if (mdfd < 0) {
map_unlock(&map);
return 1;
diff --git a/Create.c b/Create.c
index 6ca092449880..df1bc20c635b 100644
--- a/Create.c
+++ b/Create.c
@@ -605,7 +605,7 @@ int Create(struct supertype *st, char *mddev,
/* We need to create the device */
map_lock(&map);
- mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name);
+ mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name, 1);
if (mdfd < 0) {
map_unlock(&map);
return 1;
@@ -620,6 +620,7 @@ int Create(struct supertype *st, char *mddev,
chosen_name);
close(mdfd);
map_unlock(&map);
+ udev_unblock();
return 1;
}
mddev = chosen_name;
@@ -1053,9 +1054,15 @@ int Create(struct supertype *st, char *mddev,
pr_err("not starting array - not enough devices.\n");
}
close(mdfd);
+ /* Give udev a moment to process the Change event caused
+ * by the close.
+ */
+ usleep(100*1000);
+ udev_unblock();
return 0;
abort:
+ udev_unblock();
map_lock(&map);
abort_locked:
map_remove(&map, fd2devnm(mdfd));
diff --git a/Incremental.c b/Incremental.c
index 28f1f7734956..63ed4fa1a88d 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -321,7 +321,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
/* Couldn't find an existing array, maybe make a new one */
mdfd = create_mddev(match ? match->devname : NULL,
- name_to_use, c->autof, trustworthy, chosen_name);
+ name_to_use, c->autof, trustworthy, chosen_name, 0);
if (mdfd < 0)
goto out_unlock;
@@ -1605,7 +1605,7 @@ static int Incremental_container(struct supertype *st, char *devname,
ra->name,
c->autof,
trustworthy,
- chosen_name);
+ chosen_name, 0);
}
if (only && (!mp || strcmp(mp->devnm, only) != 0))
continue;
diff --git a/Makefile b/Makefile
index 685069612617..021d3adf3ed1 100644
--- a/Makefile
+++ b/Makefile
@@ -256,8 +256,8 @@ install-man: mdadm.8 md.4 mdadm.conf.5 mdmon.8
$(INSTALL) -D -m 644 md.4 $(DESTDIR)$(MAN4DIR)/md.4
$(INSTALL) -D -m 644 mdadm.conf.5 $(DESTDIR)$(MAN5DIR)/mdadm.conf.5
-install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
- @for file in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
+install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules udev-md-raid-creating.rules
+ @for file in 01-md-raid-creating.rules 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp.1 && \
$(ECHO) $(INSTALL) -D -m 644 udev-$${file#??-} $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
$(INSTALL) -D -m 644 .install.tmp.1 $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
diff --git a/lib.c b/lib.c
index b640634ef6f2..7e44b1f27fcc 100644
--- a/lib.c
+++ b/lib.c
@@ -163,6 +163,35 @@ char *fd2devnm(int fd)
return NULL;
}
+/* When we create a new array, we don't want the content to
+ * be immediately examined by udev - it is probably meaningless.
+ * So create /run/mdadm/creating-FOO and expect that a udev
+ * rule will noticed this and act accordingly.
+ */
+static char block_path[] = "/run/mdadm/creating-%s";
+static char *unblock_path = NULL;
+void udev_block(char *devnm)
+{
+ int fd;
+ char *path = NULL;
+
+ xasprintf(&path, block_path, devnm);
+ fd = open(path, O_CREAT|O_RDWR, 0600);
+ if (fd >= 0) {
+ close(fd);
+ unblock_path = path;
+ } else
+ free(path);
+}
+
+void udev_unblock(void)
+{
+ if (unblock_path)
+ unlink(unblock_path);
+ free(unblock_path);
+ unblock_path = NULL;
+}
+
/*
* convert a major/minor pair for a block device into a name in /dev, if possible.
* On the first call, walk /dev collecting name.
diff --git a/mdadm.h b/mdadm.h
index 1bbacfe9e916..6a382a7c1b90 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1533,7 +1533,7 @@ extern char *get_md_name(char *devnm);
extern char DefaultConfFile[];
extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
- char *chosen);
+ char *chosen, int block_udev);
/* values for 'trustworthy' */
#define LOCAL 1
#define LOCAL_ANY 10
@@ -1567,6 +1567,8 @@ extern char *stat2kname(struct stat *st);
extern char *fd2kname(int fd);
extern char *stat2devnm(struct stat *st);
extern char *fd2devnm(int fd);
+extern void udev_block(char *devnm);
+extern void udev_unblock(void);
extern int in_initrd(void);
diff --git a/mdopen.c b/mdopen.c
index 82b97fc90339..099efa0aa2e5 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -135,7 +135,7 @@ void make_parts(char *dev, int cnt)
*/
int create_mddev(char *dev, char *name, int autof, int trustworthy,
- char *chosen)
+ char *chosen, int block_udev)
{
int mdfd;
struct stat stb;
@@ -147,6 +147,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
char devname[37];
char devnm[32];
char cbuf[400];
+
+ if (!use_udev())
+ block_udev = 0;
+
if (chosen == NULL)
chosen = cbuf;
@@ -305,43 +309,53 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
int fd;
int n = -1;
sprintf(devnm, "md_%s", cname);
+ if (block_udev)
+ udev_block(devnm);
fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
if (fd >= 0) {
n = write(fd, devnm, strlen(devnm));
close(fd);
}
- if (n < 0)
+ if (n < 0) {
devnm[0] = 0;
+ udev_unblock();
+ }
}
if (num >= 0) {
int fd;
int n = -1;
sprintf(devnm, "md%d", num);
+ if (block_udev)
+ udev_block(devnm);
fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
if (fd >= 0) {
n = write(fd, devnm, strlen(devnm));
close(fd);
}
- if (n < 0)
+ if (n < 0) {
devnm[0] = 0;
- }
- if (devnm[0])
- ;
- else if (num < 0) {
- /* need to choose a free number. */
- char *_devnm = find_free_devnm(use_mdp);
- if (_devnm == NULL) {
- pr_err("No avail md devices - aborting\n");
- return -1;
+ udev_unblock();
}
- strcpy(devnm, _devnm);
- } else {
- sprintf(devnm, "%s%d", use_mdp?"md_d":"md", num);
- if (mddev_busy(devnm)) {
- pr_err("%s is already in use.\n",
- dev);
- return -1;
+ }
+ if (devnm[0] == 0) {
+ if (num < 0) {
+ /* need to choose a free number. */
+ char *_devnm = find_free_devnm(use_mdp);
+ if (_devnm == NULL) {
+ pr_err("No avail md devices - aborting\n");
+ return -1;
+ }
+ strcpy(devnm, _devnm);
+ } else {
+ sprintf(devnm, "%s%d", use_mdp?"md_d":"md", num);
+ if (mddev_busy(devnm)) {
+ pr_err("%s is already in use.\n",
+ dev);
+ return -1;
+ }
}
+ if (block_udev)
+ udev_block(devnm);
}
sprintf(devname, "/dev/%s", devnm);
diff --git a/udev-md-raid-creating.rules b/udev-md-raid-creating.rules
new file mode 100644
index 000000000000..2be466bcefd2
--- /dev/null
+++ b/udev-md-raid-creating.rules
@@ -0,0 +1,7 @@
+# do not edit this file, it will be overwritten on update
+# While mdadm is creating an array, it creates a file
+# /run/mdadm/creating-mdXXX. If that file exists, then
+# the array is not "ready" and we should make sure the
+# content is ignored.
+
+KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
--
2.12.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related
* Re: [MD PATCH v2 1/1] Use a new variable to count flighting sync requests
From: Xiao Ni @ 2017-04-28 5:18 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, colyli, ncroxon
In-Reply-To: <20170427210516.key5efxmifbxd3sz@kernel.org>
----- Original Message -----
> From: "Shaohua Li" <shli@kernel.org>
> To: "Xiao Ni" <xni@redhat.com>
> Cc: linux-raid@vger.kernel.org, colyli@suse.de, ncroxon@redhat.com
> Sent: Friday, April 28, 2017 5:05:16 AM
> Subject: Re: [MD PATCH v2 1/1] Use a new variable to count flighting sync requests
>
> On Thu, Apr 27, 2017 at 01:58:01PM -0700, Shaohua Li wrote:
> > On Thu, Apr 27, 2017 at 04:28:49PM +0800, Xiao Ni wrote:
> > > In new barrier codes, raise_barrier waits if conf->nr_pending[idx] is not
> > > zero.
> > > After all the conditions are true, the resync request can go on be
> > > handled. But
> > > it adds conf->nr_pending[idx] again. The next resync request hit the same
> > > bucket
> > > idx need to wait the resync request which is submitted before. The
> > > performance
> > > of resync/recovery is degraded.
> > > So we should use a new variable to count sync requests which are in
> > > flight.
> > >
> > > I did a simple test:
> > > 1. Without the patch, create a raid1 with two disks. The resync speed:
> > > Device: rrqm/s wrqm/s r/s w/s rMB/s wMB/s
> > > avgrq-sz avgqu-sz await r_await w_await svctm %util
> > > sdb 0.00 0.00 166.00 0.00 10.38 0.00
> > > 128.00 0.03 0.20 0.20 0.00 0.19 3.20
> > > sdc 0.00 0.00 0.00 166.00 0.00 10.38
> > > 128.00 0.96 5.77 0.00 5.77 5.75 95.50
> > > 2. With the patch, the result is:
> > > sdb 2214.00 0.00 766.00 0.00 185.69 0.00
> > > 496.46 2.80 3.66 3.66 0.00 1.03 79.10
> > > sdc 0.00 2205.00 0.00 769.00 0.00 186.44
> > > 496.52 5.25 6.84 0.00 6.84 1.30 100.10
> > >
> > > Suggested-by: Shaohua Li <shli@kernel.org>
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> >
> > applied, thanks!
> > > ---
> > > drivers/md/raid1.c | 5 +++--
> > > drivers/md/raid1.h | 1 +
> > > 2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > > index a34f587..ff5ee53 100644
> > > --- a/drivers/md/raid1.c
> > > +++ b/drivers/md/raid1.c
> > > @@ -869,7 +869,7 @@ static void raise_barrier(struct r1conf *conf,
> > > sector_t sector_nr)
> > > atomic_read(&conf->barrier[idx]) < RESYNC_DEPTH,
> > > conf->resync_lock);
> > >
> > > - atomic_inc(&conf->nr_pending[idx]);
> > > + atomic_inc(&conf->nr_sync_pending);
> > > spin_unlock_irq(&conf->resync_lock);
> > > }
> > >
> > > @@ -880,7 +880,7 @@ static void lower_barrier(struct r1conf *conf,
> > > sector_t sector_nr)
> > > BUG_ON(atomic_read(&conf->barrier[idx]) <= 0);
> > >
> > > atomic_dec(&conf->barrier[idx]);
> > > - atomic_dec(&conf->nr_pending[idx]);
> > > + atomic_dec(&conf->nr_sync_pending);
> > > wake_up(&conf->wait_barrier);
> > > }
> > >
> > > @@ -1017,6 +1017,7 @@ static int get_unqueued_pending(struct r1conf
> > > *conf)
> > > {
> > > int idx, ret;
> > >
> > > + ret = atomic_read(&conf->nr_sync_pending);
> > > for (ret = 0, idx = 0; idx < BARRIER_BUCKETS_NR; idx++)
>
> actually I deleted the 'ret = 0'
Sorry, I didn't notice this. I need more attention. And thanks
for the modification.
Xiao
>
> > > ret += atomic_read(&conf->nr_pending[idx]) -
> > > atomic_read(&conf->nr_queued[idx]);
> > > diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> > > index dd22a37..1668f22 100644
> > > --- a/drivers/md/raid1.h
> > > +++ b/drivers/md/raid1.h
> > > @@ -84,6 +84,7 @@ struct r1conf {
> > > */
> > > wait_queue_head_t wait_barrier;
> > > spinlock_t resync_lock;
> > > + atomic_t nr_sync_pending;
> > > atomic_t *nr_pending;
> > > atomic_t *nr_waiting;
> > > atomic_t *nr_queued;
> > > --
> > > 2.7.4
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function
From: Herbert Xu @ 2017-04-28 6:30 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
target-devel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, James E.J. Bottomley,
linux-scsi-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Sumit Semwal,
open-iscsi-/JYPxA39Uh5TLH3MbocFFw,
linux-media-u79uwXL29TY76Z2rM5mHXA,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
sparmaintainer-GLv8BlqOqDDQT0dZR+AlfA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
megaraidlinux.pdl-dY08KVG/lbpWk0Htik3J/w, Jens Axboe,
Martin K. Petersen, netdev-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman,
David S. Miller
In-Reply-To: <94123cbf-3287-f05e-7267-0bcf08ab0a8b-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
On Thu, Apr 27, 2017 at 09:45:57AM -0600, Logan Gunthorpe wrote:
>
>
> On 26/04/17 09:56 PM, Herbert Xu wrote:
> > On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> >> Very straightforward conversion to the new function in the caam driver
> >> and shash library.
> >>
> >> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
> >> Cc: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
> >> Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> >> ---
> >> crypto/shash.c | 9 ++++++---
> >> drivers/crypto/caam/caamalg.c | 8 +++-----
> >> 2 files changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/crypto/shash.c b/crypto/shash.c
> >> index 5e31c8d..5914881 100644
> >> --- a/crypto/shash.c
> >> +++ b/crypto/shash.c
> >> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
> >> if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
> >> void *data;
> >>
> >> - data = kmap_atomic(sg_page(sg));
> >> - err = crypto_shash_digest(desc, data + offset, nbytes,
> >> + data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> >> + if (IS_ERR(data))
> >> + return PTR_ERR(data);
> >> +
> >> + err = crypto_shash_digest(desc, data, nbytes,
> >> req->result);
> >> - kunmap_atomic(data);
> >> + sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
> >> crypto_yield(desc->flags);
> >> } else
> >> err = crypto_shash_init(desc) ?:
> >
> > Nack. This is an optimisation for the special case of a single
> > SG list entry. In fact in the common case the kmap_atomic should
> > disappear altogether in the no-highmem case. So replacing it
> > with sg_map is not acceptable.
>
> What you seem to have missed is that sg_map is just a thin wrapper
> around kmap_atomic. Perhaps with a future check for a mappable page.
> This change should have zero impact on performance.
You are right. Indeed the existing code looks buggy as they
don't take sg->offset into account when doing the kmap. Could
you send me some patches that fix these problems first so that
they can be easily backported?
Thanks,
--
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Recovery on new 2TB disk: finish=7248.4min (raid1)
From: Ron Leach @ 2017-04-28 7:05 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <e76a0a07-2306-e63a-a1f7-48d103eefd62@thelounge.net>
On 27/04/2017 15:43, Reindl Harald wrote:
> [root@rh:~]$ cat /proc/mdstat
> Personalities : [raid10] [raid1]
> md0 : active raid1 sda1[0] sdc1[1] sdb1[3] sdd1[2]
> 511988 blocks super 1.0 [4/4] [UUUU]
>
> md1 : active raid10 sda2[0] sdc2[1] sdd2[2] sdb2[3]
> 30716928 blocks super 1.1 512K chunks 2 near-copies [4/4] [UUUU]
>
> md2 : active raid10 sda3[0] sdc3[1] sdd3[2] sdb3[3]
> 3875222528 blocks super 1.1 512K chunks 2 near-copies [4/4] [UUUU]
> [========>............] check = 44.4% (1721204032/3875222528)
> finish=470.9min speed=76232K/sec
>
Those were the sort of times that I used to see on this machine.
I've fixed it now, though. There were some clues in syslog - gdm3 was
alerting 2 or 3 times each second, continually. This was because I'd
taken this server offline and across to a workbench to change the
disk. I'd restarted the machine, partitioned it, etc, and issued
those --add commands, without a screen or keyboard, just over ssh. I
hadn't realised that gdm3 would panic, causing a couple of acpid
messages as well each time.
Someone on the Debian list pointed out that gdm3 was a service and
could be stopped for this circumstance. Doing that seemed to release
mdadm to recovering at its normal rate; all the mds are fully
replicated, now.
Thanks to folks for contributing their thoughts, some interesting
insights came up as well which will be useful in the future. This is
quite an old server (still actively used), created before I realised
the drawbacks of having so many partitions for each part of the
filesystem; I don't do this on more-recent systems, which look more
like the setup you show here.
Anyway, all seems ok now, and thanks again,
regards, Ron
^ permalink raw reply
* Re: Recovery on new 2TB disk: finish=7248.4min (raid1)
From: Mateusz Korniak @ 2017-04-28 7:06 UTC (permalink / raw)
To: Ron Leach; +Cc: linux-raid
In-Reply-To: <59024018.8060006@tesco.net>
On Thursday 27 of April 2017 20:01:44 Ron Leach wrote:
> > iostat -x 30 2 -m
>
> What would the command have done?
It would hint performance and load of block devices in system.
--
Mateusz Korniak
"(...) mam brata - poważny, domator, liczykrupa, hipokryta, pobożniś,
krótko mówiąc - podpora społeczeństwa."
Nikos Kazantzakis - "Grek Zorba"
^ permalink raw reply
* Re: Recover array after I panicked
From: Patrik Dahlström @ 2017-04-28 7:11 UTC (permalink / raw)
To: Andreas Klauer; +Cc: Brad Campbell, linux-raid
In-Reply-To: <20170427231242.GA5138@metamorpher.de>
2017-04-28 1:12 GMT+02:00, Andreas Klauer <Andreas.Klauer@metamorpher.de>:
> On Thu, Apr 27, 2017 at 09:57:20PM +0200, Patrik Dahlström wrote:
>> Success! Using a 126M as data offset gave me valid data for the 5 disk
>> raid and using linear device mapping I'm able to access my data again.
>
> Nice.
>
>> Some is probably corrupted from my previous destructive recovery
>> attempts, but it seems like most of the data is still there and
>> accessible. I will start backup up the most essential data now.
>
> If nothing else happened, I'd expect it to be intact (or at least
> not any more corrupt than it would be after a regular power loss).
> If you encounter any files with corrupted contents, you could use
> `filefrag` or `hdparm --fibmap` to determine where it is physically
> located and then perhaps see if that's anywhere near your danger zone.
> It could still mean you didn't choose the correct X for linear mapping.
>
> But of course, I don't know everything that happened. :-P
During some of my experiments, reshaping was triggered with wrong
metadata. This corrupted my data. I'm gonna ignore this and let fsck
solve it. I already tried it with overlays and linear device mapping.
>
>> > This is only a way to get read access at the data, making the raid
>> > work as a standalone again (resume reshape with lost raid metadata)
>> > is another problem, to be tackled after you backed up your data ;)
>>
>> Now, how do we do this?
>
> Well, there's an elegant approach that works beautifully and perfectly
> and entirely risk free... and then there's a simple, stupid, quick&dirty.
> Only problem is, I'm way too lazy to describe the elegant one. Shucks.
Ha!
>
> The quick and dirty method is... grow using dd from 5disk raid (overlay)
> to 6disk raid (no overlay) starting from the correct offset (verify!).
> That resumes your reshape in an offline-ish, hackish manner, and once
> it's done you have the whole thing. That's the theory anyway.
>
> Pseudocode:
>
> mdadm --create /dev/md5 --assume-clean /dev/overlay/{a,b,c}
> mdadm --create /dev/md6 --assume-clean /dev/{a,b,c,d} # +1 drive
> losetup --offset=X /dev/loop55 /dev/md5
> losetup --offset=X /dev/loop66 /dev/md6
> cmp /dev/loop55 /dev/loop66 # much identical, so wow
> dd bs=1M status=progress if=/dev/loop55 of=/dev/loop66
Is it missing a 'skip=<offset/1M>' here?
>
> It's dangerous, okay?
> I didn't tell you to do it, okay?
> Don't blame me, okay?
> Backup your stuff first, okay?
Ha!
>
> As this writes to your drives, you have only one shot to get it right.
> You're not allowed to mount until dd is done. It's an offline operation.
> If dd were to be aborted for any reason, the offset would shift to X+n.
> If dd were to encounter write errors without aborting, corruption ensues.
Swell!
>
> Regards
> Andreas Klauer
>
Thanks a lot. I will try this later.
^ permalink raw reply
* Re: Recovery on new 2TB disk: finish=7248.4min (raid1)
From: Mateusz Korniak @ 2017-04-28 7:30 UTC (permalink / raw)
To: Reindl Harald; +Cc: John Stoffel, Ron Leach, linux-raid
In-Reply-To: <b74c3cae-a55d-9306-16fe-5b811c20ec89@thelounge.net>
On Thursday 27 of April 2017 21:42:27 Reindl Harald wrote:
> > Just create a single MD device across the entire disk (...)
>
> but you can't boot from a RAID5/RAID6/RAID10
Since some time you can boot from LVM LV having PV(s) on RAID10 with grub2.
--
Mateusz Korniak
"(...) mam brata - poważny, domator, liczykrupa, hipokryta, pobożniś,
krótko mówiąc - podpora społeczeństwa."
Nikos Kazantzakis - "Grek Zorba"
^ permalink raw reply
* Re: [mdadm PATCH 4/4] Create: tell udev device is not ready when first created.
From: Peter Rajnoha @ 2017-04-28 9:08 UTC (permalink / raw)
To: NeilBrown, Jes Sorensen; +Cc: linux-raid, dm-devel
In-Reply-To: <87k265rxsm.fsf@notabene.neil.brown.name>
On 04/28/2017 05:55 AM, NeilBrown wrote:
> On Wed, Apr 26 2017, Peter Rajnoha wrote:
>
>> On 04/20/2017 11:35 PM, NeilBrown wrote:
>>> If we wanted an more permanent udev rule, we would need to record the
>>> devices that should be ignored in the filesystem somewhere else.
>>> Maybe in /run/mdadm.
>>> e.g.
>>>
>>> KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
>>>
>>> Then we could have something like the following (untested) in mdadm.
>>> Does that seem more suitable?
>>>
>>
>> Yes, please, if possible, go for a permanent udev rule surely - this
>> will make it much easier for other foreign tools to hook in properly if
>> needed and it would also be much easier to debug.
>
> I'm leaning towards the files-in-/run/mdadm approach too. I'll make a
> proper patch.
>
>>
>> But, wouldn't it be better if we could just pass this information ("not
>> initialized yet") as RUN_ARRAY md ioctl parameter? In that case the md
>> driver in kernel could add the variable to the uevent it generates which
>> userspace udev rules could check for easily. This way, we don't need to
>> hassle with creating files in filesystem and the information would be
>> directly a part of the uevent the md kernel driver generates (so
>> directly accessible in udev rules too). Also, possibly adding more
>> variables for other future scenarios if needed.
>
> When would we clear the "not initialised yet" flag in the kernel, and
> how? And would that be enough.
The flag wouldn't be stored in kernel, md kernel driver would just pass
the flag with the uevent as it received in with ioctl/sysfs request to
create a new dev. The udev in userspace would handle the state
transition then from "flagged as not-initialized" state to "initilized"
by checking the sequence of events as they come.
We should have this sequence I assume:
1) "add" (creating dev, not usable yet)
2) "change" (activated dev, but not initialized yet)
3) "synthetic change" (after wiping the dev and closing it, the WATCH
rule fires)
>
> When mdadm creates an md array, at least 3 uevents get generated.
> The first is generated when the md device object is created, either by
> opening the /dev/mdXX file, or by writing magic to somewhere in sysfs.
> The second is generated when the array is started and the content is
> visible.
> The third is generated when mdadm closes the file descriptor. It opens
> /dev/mdXX for O_RDWR and performs ioctls on this, and then closes it.
> Because udev uses inotify to watch for a 'close for a writable file
> descriptor', this generates another event.
>
> We need to ensure that none of these cause udev to run anything that
> inspects the contents of the array.
> Of the three, only the second one is directly under the control of the
> md module, so only that one can add an environment variable.
>
> It might be possible to avoid the last one (by not opening for write),
> but I cannot see a way to avoid the first one.
So the first event is the "add" event ("md device object created") - in
this case, the device is not yet usable anyway I suppose, so we can skip
udev scans for this event right away (unless it's the synthetic "add"
event which is generated by writing "add" to /sys/block/.../uevent file
or alternatively using udevadm trigger - but we should be able to
recognize this event "synthetic add event" because it always comes after
the activating "change" event, otherwise we can skip scans).
The second event, which is the "change" event, would be marked with the
new "not initialized" flag. And so we skip the scans in udev too.
Then mdadm opens the devive, clears any old content/signatures the data
area may contain, then closes it - this generates the third event -
which is the "synthetic change" event (as a result of the inotify WATCH
rule). And this one would drop the "not initialized" flag in udev db and
the scans in udev are now enabled.
So we should be able to handle all three kinds of events I think.
Now, as for even better synthetic event recognition, I've proposed a
patch recently where udev as well as any other tool generating these
synthetic events can add variables for more detailed event
identification, so this one should help us in the future even more in
these situations: https://lkml.org/lkml/2017/3/15/461. With this, we can
even disable the WATCH rule till the device is properly initialized and
the tool can generate the event itself by writing the
/sys/block/.../uevent file with a variable that the tool can then wait
for even (so the tool doesn't exit till the device is not properly
initialized). Once this initialization is all done, the WATCH rule can
be enabled for the dev. Also, with this, we don't need to be afraid that
some other tool fired the WATCH rule by chance if it opened the dev for
RW and closed it by mistake before we had a chance to initialize it
(which would fire the synthetic change event before the
wiping/initialization).
>
> I don't think that making a file appear in /run is really very different
> from making a variable appear in a uevent. If the variable were
> describing the event itself there would be a different, but it would be
> describing the state of the device. So the only important difference is
> "which is easier to work with". I think creating an deleting a file is
> easier to setting and clearing a variable.
>
Yes, I agree that it's an alternative solution - it definitely and
surely improves current situation, either if we choose to write the file
or pass the flag in the uevent directly. It's just that with the
information written in filesystem, we have something external to check
for in addition to processing the uevent variables while we don't need
to, I think. As I described the sequence of events above, I think we
should be able to recognize the events properly and we should be able to
drop the flag automatically.
--
Peter
^ permalink raw reply
* Re: [mdadm PATCH] Create: tell udev md device is not ready when first created.
From: Peter Rajnoha @ 2017-04-28 9:28 UTC (permalink / raw)
To: NeilBrown, Jes Sorensen; +Cc: linux-raid, dm-devel
In-Reply-To: <87h919ruj5.fsf@notabene.neil.brown.name>
On 04/28/2017 07:05 AM, NeilBrown wrote:
>
> When an array is created the content is not initialized,
> so it could have remnants of an old filesystem or md array
> etc on it.
> udev will see this and might try to activate it, which is almost
> certainly not what is wanted.
>
> So create a mechanism for mdadm to communicate with udev to tell
> it that the device isn't ready. This mechanism is the existance
> of a file /run/mdadm/created-mdXXX where mdXXX is the md device name.
>
> When creating an array, mdadm will create the file.
> A new udev rule file, 01-md-raid-creating.rules, will detect the
> precense of thst file and set ENV{SYSTEMD_READY}="0".
> This is fairly uniformly used to suppress actions based on the
> contents of the device.
>
The scans in udev are primarily directed by blkid call which detects the
signatures and based on this information various other udev rules fire.
The blkid as well as wipefs uses common libblkid library to detect these
signatures - is mdadm going to use libblkid to wipe the signatures on MD
device initialization or is it relying on external tools to do this? How
is mdadm actually initializing/wiping the newly created MD device?
--
Peter
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> Assemble.c | 2 +-
> Build.c | 2 +-
> Create.c | 9 +++++++-
> Incremental.c | 4 ++--
> Makefile | 4 ++--
> lib.c | 29 +++++++++++++++++++++++++
> mdadm.h | 4 +++-
> mdopen.c | 52 ++++++++++++++++++++++++++++-----------------
> udev-md-raid-creating.rules | 7 ++++++
> 9 files changed, 86 insertions(+), 27 deletions(-)
> create mode 100644 udev-md-raid-creating.rules
>
> diff --git a/Assemble.c b/Assemble.c
> index d6beb23da9c5..a9442c8ce73b 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -1478,7 +1478,7 @@ try_again:
> name = strchr(name, ':')+1;
>
> mdfd = create_mddev(mddev, name, ident->autof, trustworthy,
> - chosen_name);
> + chosen_name, 0);
> }
> if (mdfd < 0) {
> st->ss->free_super(st);
> diff --git a/Build.c b/Build.c
> index 11ba12f4ae7d..665d9067b8d6 100644
> --- a/Build.c
> +++ b/Build.c
> @@ -109,7 +109,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
> /* We need to create the device. It can have no name. */
> map_lock(&map);
> mdfd = create_mddev(mddev, NULL, c->autof, LOCAL,
> - chosen_name);
> + chosen_name, 0);
> if (mdfd < 0) {
> map_unlock(&map);
> return 1;
> diff --git a/Create.c b/Create.c
> index 6ca092449880..df1bc20c635b 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -605,7 +605,7 @@ int Create(struct supertype *st, char *mddev,
>
> /* We need to create the device */
> map_lock(&map);
> - mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name);
> + mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name, 1);
> if (mdfd < 0) {
> map_unlock(&map);
> return 1;
> @@ -620,6 +620,7 @@ int Create(struct supertype *st, char *mddev,
> chosen_name);
> close(mdfd);
> map_unlock(&map);
> + udev_unblock();
> return 1;
> }
> mddev = chosen_name;
> @@ -1053,9 +1054,15 @@ int Create(struct supertype *st, char *mddev,
> pr_err("not starting array - not enough devices.\n");
> }
> close(mdfd);
> + /* Give udev a moment to process the Change event caused
> + * by the close.
> + */
> + usleep(100*1000);
> + udev_unblock();
> return 0;
>
> abort:
> + udev_unblock();
> map_lock(&map);
> abort_locked:
> map_remove(&map, fd2devnm(mdfd));
> diff --git a/Incremental.c b/Incremental.c
> index 28f1f7734956..63ed4fa1a88d 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -321,7 +321,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
>
> /* Couldn't find an existing array, maybe make a new one */
> mdfd = create_mddev(match ? match->devname : NULL,
> - name_to_use, c->autof, trustworthy, chosen_name);
> + name_to_use, c->autof, trustworthy, chosen_name, 0);
>
> if (mdfd < 0)
> goto out_unlock;
> @@ -1605,7 +1605,7 @@ static int Incremental_container(struct supertype *st, char *devname,
> ra->name,
> c->autof,
> trustworthy,
> - chosen_name);
> + chosen_name, 0);
> }
> if (only && (!mp || strcmp(mp->devnm, only) != 0))
> continue;
> diff --git a/Makefile b/Makefile
> index 685069612617..021d3adf3ed1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -256,8 +256,8 @@ install-man: mdadm.8 md.4 mdadm.conf.5 mdmon.8
> $(INSTALL) -D -m 644 md.4 $(DESTDIR)$(MAN4DIR)/md.4
> $(INSTALL) -D -m 644 mdadm.conf.5 $(DESTDIR)$(MAN5DIR)/mdadm.conf.5
>
> -install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
> - @for file in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
> +install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules udev-md-raid-creating.rules
> + @for file in 01-md-raid-creating.rules 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
> do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp.1 && \
> $(ECHO) $(INSTALL) -D -m 644 udev-$${file#??-} $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
> $(INSTALL) -D -m 644 .install.tmp.1 $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
> diff --git a/lib.c b/lib.c
> index b640634ef6f2..7e44b1f27fcc 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -163,6 +163,35 @@ char *fd2devnm(int fd)
> return NULL;
> }
>
> +/* When we create a new array, we don't want the content to
> + * be immediately examined by udev - it is probably meaningless.
> + * So create /run/mdadm/creating-FOO and expect that a udev
> + * rule will noticed this and act accordingly.
> + */
> +static char block_path[] = "/run/mdadm/creating-%s";
> +static char *unblock_path = NULL;
> +void udev_block(char *devnm)
> +{
> + int fd;
> + char *path = NULL;
> +
> + xasprintf(&path, block_path, devnm);
> + fd = open(path, O_CREAT|O_RDWR, 0600);
> + if (fd >= 0) {
> + close(fd);
> + unblock_path = path;
> + } else
> + free(path);
> +}
> +
> +void udev_unblock(void)
> +{
> + if (unblock_path)
> + unlink(unblock_path);
> + free(unblock_path);
> + unblock_path = NULL;
> +}
> +
> /*
> * convert a major/minor pair for a block device into a name in /dev, if possible.
> * On the first call, walk /dev collecting name.
> diff --git a/mdadm.h b/mdadm.h
> index 1bbacfe9e916..6a382a7c1b90 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1533,7 +1533,7 @@ extern char *get_md_name(char *devnm);
> extern char DefaultConfFile[];
>
> extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
> - char *chosen);
> + char *chosen, int block_udev);
> /* values for 'trustworthy' */
> #define LOCAL 1
> #define LOCAL_ANY 10
> @@ -1567,6 +1567,8 @@ extern char *stat2kname(struct stat *st);
> extern char *fd2kname(int fd);
> extern char *stat2devnm(struct stat *st);
> extern char *fd2devnm(int fd);
> +extern void udev_block(char *devnm);
> +extern void udev_unblock(void);
>
> extern int in_initrd(void);
>
> diff --git a/mdopen.c b/mdopen.c
> index 82b97fc90339..099efa0aa2e5 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -135,7 +135,7 @@ void make_parts(char *dev, int cnt)
> */
>
> int create_mddev(char *dev, char *name, int autof, int trustworthy,
> - char *chosen)
> + char *chosen, int block_udev)
> {
> int mdfd;
> struct stat stb;
> @@ -147,6 +147,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> char devname[37];
> char devnm[32];
> char cbuf[400];
> +
> + if (!use_udev())
> + block_udev = 0;
> +
> if (chosen == NULL)
> chosen = cbuf;
>
> @@ -305,43 +309,53 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> int fd;
> int n = -1;
> sprintf(devnm, "md_%s", cname);
> + if (block_udev)
> + udev_block(devnm);
> fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> if (fd >= 0) {
> n = write(fd, devnm, strlen(devnm));
> close(fd);
> }
> - if (n < 0)
> + if (n < 0) {
> devnm[0] = 0;
> + udev_unblock();
> + }
> }
> if (num >= 0) {
> int fd;
> int n = -1;
> sprintf(devnm, "md%d", num);
> + if (block_udev)
> + udev_block(devnm);
> fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> if (fd >= 0) {
> n = write(fd, devnm, strlen(devnm));
> close(fd);
> }
> - if (n < 0)
> + if (n < 0) {
> devnm[0] = 0;
> - }
> - if (devnm[0])
> - ;
> - else if (num < 0) {
> - /* need to choose a free number. */
> - char *_devnm = find_free_devnm(use_mdp);
> - if (_devnm == NULL) {
> - pr_err("No avail md devices - aborting\n");
> - return -1;
> + udev_unblock();
> }
> - strcpy(devnm, _devnm);
> - } else {
> - sprintf(devnm, "%s%d", use_mdp?"md_d":"md", num);
> - if (mddev_busy(devnm)) {
> - pr_err("%s is already in use.\n",
> - dev);
> - return -1;
> + }
> + if (devnm[0] == 0) {
> + if (num < 0) {
> + /* need to choose a free number. */
> + char *_devnm = find_free_devnm(use_mdp);
> + if (_devnm == NULL) {
> + pr_err("No avail md devices - aborting\n");
> + return -1;
> + }
> + strcpy(devnm, _devnm);
> + } else {
> + sprintf(devnm, "%s%d", use_mdp?"md_d":"md", num);
> + if (mddev_busy(devnm)) {
> + pr_err("%s is already in use.\n",
> + dev);
> + return -1;
> + }
> }
> + if (block_udev)
> + udev_block(devnm);
> }
>
> sprintf(devname, "/dev/%s", devnm);
> diff --git a/udev-md-raid-creating.rules b/udev-md-raid-creating.rules
> new file mode 100644
> index 000000000000..2be466bcefd2
> --- /dev/null
> +++ b/udev-md-raid-creating.rules
> @@ -0,0 +1,7 @@
> +# do not edit this file, it will be overwritten on update
> +# While mdadm is creating an array, it creates a file
> +# /run/mdadm/creating-mdXXX. If that file exists, then
> +# the array is not "ready" and we should make sure the
> +# content is ignored.
> +
> +KERNEL=="md*", TEST="/run/mdadm/creating-$kernel", ENV{SYSTEMD_READY}="0"
>
^ permalink raw reply
* Re: Recover array after I panicked
From: Andreas Klauer @ 2017-04-28 9:52 UTC (permalink / raw)
To: Patrik Dahlström; +Cc: Brad Campbell, linux-raid
In-Reply-To: <CAHKno4DBXT5n7wYosie_WHBhzXnHj0CFkdjv7_VAN2NVF-Ng4g@mail.gmail.com>
On Fri, Apr 28, 2017 at 09:11:45AM +0200, Patrik Dahlström wrote:
> Is it missing a 'skip=<offset/1M>' here?
In this example the offset is provided by the loop devices.
If you don't like loop devices you can use seek=X skip=X (same X)
but you should triple check the offset is correct, data at this
offset should be identical on both arrays. (What my example
checks using `cmp` which should return differ at byte xxxxxxxxx.
Have a look at it with hexdump too so it's not just zeroes...)
Regards
Andreas Klauer
^ permalink raw reply
* Re: Recover array after I panicked
From: Patrik Dahlström @ 2017-04-28 10:31 UTC (permalink / raw)
To: Andreas Klauer; +Cc: Brad Campbell, linux-raid
In-Reply-To: <20170428095252.GA3793@metamorpher.de>
2017-04-28 11:52 GMT+02:00, Andreas Klauer <Andreas.Klauer@metamorpher.de>:
> On Fri, Apr 28, 2017 at 09:11:45AM +0200, Patrik Dahlström wrote:
>> Is it missing a 'skip=<offset/1M>' here?
>
> In this example the offset is provided by the loop devices.
>
Yes, I saw that after I sent my reply.
Can't I reduce my risks by doing it the other way around? Restore the
5 disk raid and then restart the reshape?
dd if=/dev/md6 bs=1M count=X/1M of=/dev/md5
mdadm --grow --raid-devices=6 /dev/md5 --add /dev/sdf
--backup-file=/root/grow_md5.bak
It's a bit more disk I/O, but less risk
^ permalink raw reply
* Re: Recover array after I panicked
From: Andreas Klauer @ 2017-04-28 11:39 UTC (permalink / raw)
To: Patrik Dahlström; +Cc: Brad Campbell, linux-raid
In-Reply-To: <CAHKno4DWXfOw-ejU5zfh_A8WiG7haSafNTbJLM4Ue1oTCFksyw@mail.gmail.com>
On Fri, Apr 28, 2017 at 12:31:43PM +0200, Patrik Dahlström wrote:
> Can't I reduce my risks by doing it the other way around?
Not like this.
> dd if=/dev/md6 bs=1M count=X/1M of=/dev/md5
Writes into data you haven't read yet, so you end up reading data
you just wrote and write that again... the result will be garbage.
ddrescue has a reverse mode but implementation details matter a lot.
It might be tempting because your progress is just 10-20%ish but
going forwards is a lot safer here.
If you must go backwards you should use mdadm's revert-reshape
for which you need RAID metadata that properly represents the
current mid-grow state of your RAID (the elegant approach).
You can produce such metadata by
truncate -s <drivesize> a b c d e f
losetup --find -show
mdadm --create 5disk
mdadm --grow 6disk
<fallocate punchhole in the background>
<adapt sync_speed_min/max in the background>
<watch progress until X>
mdadm --stop
mdadm --examine <verify correct offsets and reshape pos'n>
Tadaa.
Regards
Andreas Klauer
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox