FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 07-06-2012, 07:49 PM
Mike Snitzer
 
Default dm-mirror: fix crash with mirror recovery and discard

On Fri, Jul 06 2012 at 2:56pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> dm-mirror: fix crash with mirror recovery and discard
...
> These bypasses were improperly implemented for REQ_DISCARD. In
> do_writes, REQ_DISCARD requests is always added on sync queue and
> immediatelly dispatched (even if the region is in DM_RH_RECOVERING).
> However, dm_rh_inc and dm_rh_dec is called for REQ_DISCARD resusts.
> So it violates the rule that no I/Os are started on DM_RH_RECOVERING
> regions, and causes the list corruption described above.

Yeap, my fault. Thanks for sorting this out. So this bug has existed
since: 5fc2ffe v2.6.38-rc1 dm raid1: support discard

Best to reference that in the patch header so stable knows how far back
this issue goes.

> This patch changes it so that REQ_DISCARD requests follow the same path
> as REQ_FLUSH. This avoids the crash.
>
> Note that we can't guarantee that REQ_DISCARD zeroes the data even if
> the underlying disks support zero on discard, so this patch also sets
> ti->discard_zeroes_data_unsupported.

...

> Index: linux-3.4.4-fast/drivers/md/dm-raid1.c
> ================================================== =================
> --- linux-3.4.4-fast.orig/drivers/md/dm-raid1.c 2012-07-06 19:01:27.000000000 +0200
> +++ linux-3.4.4-fast/drivers/md/dm-raid1.c 2012-07-06 19:03:53.000000000 +0200
> @@ -1091,6 +1091,7 @@ static int mirror_ctr(struct dm_target *
>
> ti->num_flush_requests = 1;
> ti->num_discard_requests = 1;
> + ti->discard_zeroes_data_unsupported = 1;
>
> ms->kmirrord_wq = alloc_workqueue("kmirrord",
> WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);

This should be split out to a separate patch and properly justified in
the patch header. Is there something unique to dm-mirror that renders
the underlying device's zeroing unreliable?

> Index: linux-3.4.4-fast/drivers/md/dm-region-hash.c
> ================================================== =================
> --- linux-3.4.4-fast.orig/drivers/md/dm-region-hash.c 2012-07-06 19:02:28.000000000 +0200
> +++ linux-3.4.4-fast/drivers/md/dm-region-hash.c 2012-07-06 19:02:57.000000000 +0200
> @@ -524,7 +527,7 @@ void dm_rh_inc_pending(struct dm_region_
> struct bio *bio;
>
> for (bio = bios->head; bio; bio = bio->bi_next) {
> - if (bio->bi_rw & REQ_FLUSH)
> + if (bio->bi_rw & (REQ_FLUSH | REQ_FLUSH))
> continue;
> rh_inc(rh, dm_rh_bio_to_region(rh, bio));
> }

Typo, you meant: if (bio->bi_rw & (REQ_FLUSH | REQ_DISCARD))

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-06-2012, 08:38 PM
Mikulas Patocka
 
Default dm-mirror: fix crash with mirror recovery and discard

> > + ti->discard_zeroes_data_unsupported = 1;
> >
> > ms->kmirrord_wq = alloc_workqueue("kmirrord",
> > WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
>
> This should be split out to a separate patch and properly justified in
> the patch header. Is there something unique to dm-mirror that renders
> the underlying device's zeroing unreliable?

There are two possible approaches to handling REQ_DISCARD

1. treat REQ_DISCARD as REQ_FLUSH (this is what the patch does) --- i.e.
do not synchronize it with region states, do not set mirror error on
failure. In this mode we must assume that there are uninitialized data
after a flush.

For example, if there is a region that is being resynchronized and we send
REQ_DISCARD that overlaps this region, there is no guarantee that data in
this region were zeroed.

- kcopyd reads a few blocks for resynchronization
- REQ_DISCARD is sent to both mirror legs, both disks overwrites the area
with zeroes
- kcopyd writes those blocks to the other leg => the blocks are no longer
zero despite REQ_DISCARD being sent


2. treat REQ_DISCARD as writes (i.e. synchronize it with region states,
wait until resynchronization finishes, etc.) --- it is possible to do it
this way to, but if we do it this way, we have to split REQ_DISCARD on
region boundaries (it is currently split only on target boundaries,
which is insufficient).


Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-06-2012, 08:44 PM
Mikulas Patocka
 
Default dm-mirror: fix crash with mirror recovery and discard

On Fri, 6 Jul 2012, Mikulas Patocka wrote:

> > > + ti->discard_zeroes_data_unsupported = 1;
> > >
> > > ms->kmirrord_wq = alloc_workqueue("kmirrord",
> > > WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
> >
> > This should be split out to a separate patch and properly justified in
> > the patch header. Is there something unique to dm-mirror that renders
> > the underlying device's zeroing unreliable?
>
> There are two possible approaches to handling REQ_DISCARD
>
> 1. treat REQ_DISCARD as REQ_FLUSH (this is what the patch does) --- i.e.
> do not synchronize it with region states, do not set mirror error on
> failure. In this mode we must assume that there are uninitialized data
> after a flush.

s/flush/discard/

> For example, if there is a region that is being resynchronized and we send
> REQ_DISCARD that overlaps this region, there is no guarantee that data in
> this region were zeroed.
>
> - kcopyd reads a few blocks for resynchronization
> - REQ_DISCARD is sent to both mirror legs, both disks overwrites the area
> with zeroes
> - kcopyd writes those blocks to the other leg => the blocks are no longer
> zero despite REQ_DISCARD being sent
>
>
> 2. treat REQ_DISCARD as writes (i.e. synchronize it with region states,
> wait until resynchronization finishes, etc.) --- it is possible to do it
> this way to, but if we do it this way, we have to split REQ_DISCARD on
> region boundaries (it is currently split only on target boundaries,
> which is insufficient).
>
>
> Mikulas
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-09-2012, 12:16 PM
Mike Snitzer
 
Default dm-mirror: fix crash with mirror recovery and discard

On Fri, Jul 06 2012 at 4:38pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> > > + ti->discard_zeroes_data_unsupported = 1;
> > >
> > > ms->kmirrord_wq = alloc_workqueue("kmirrord",
> > > WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
> >
> > This should be split out to a separate patch and properly justified in
> > the patch header. Is there something unique to dm-mirror that renders
> > the underlying device's zeroing unreliable?
>
> There are two possible approaches to handling REQ_DISCARD
>
> 1. treat REQ_DISCARD as REQ_FLUSH (this is what the patch does) --- i.e.
> do not synchronize it with region states, do not set mirror error on
> failure. In this mode we must assume that there are uninitialized data
> after a flush.
>
> For example, if there is a region that is being resynchronized and we send
> REQ_DISCARD that overlaps this region, there is no guarantee that data in
> this region were zeroed.
>
> - kcopyd reads a few blocks for resynchronization
> - REQ_DISCARD is sent to both mirror legs, both disks overwrites the area
> with zeroes
> - kcopyd writes those blocks to the other leg => the blocks are no longer
> zero despite REQ_DISCARD being sent

OK, thanks, but my point still stands: this is worthy of a separate
patch (with the same type of backround you provided above).

> 2. treat REQ_DISCARD as writes (i.e. synchronize it with region states,
> wait until resynchronization finishes, etc.) --- it is possible to do it
> this way to, but if we do it this way, we have to split REQ_DISCARD on
> region boundaries (it is currently split only on target boundaries,
> which is insufficient).

I think discards should be treated as writes.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-09-2012, 11:12 PM
Mikulas Patocka
 
Default dm-mirror: fix crash with mirror recovery and discard

On Mon, 9 Jul 2012, Mike Snitzer wrote:

> On Fri, Jul 06 2012 at 4:38pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> > > > + ti->discard_zeroes_data_unsupported = 1;
> > > >
> > > > ms->kmirrord_wq = alloc_workqueue("kmirrord",
> > > > WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
> > >
> > > This should be split out to a separate patch and properly justified in
> > > the patch header. Is there something unique to dm-mirror that renders
> > > the underlying device's zeroing unreliable?
> >
> > There are two possible approaches to handling REQ_DISCARD
> >
> > 1. treat REQ_DISCARD as REQ_FLUSH (this is what the patch does) --- i.e.
> > do not synchronize it with region states, do not set mirror error on
> > failure. In this mode we must assume that there are uninitialized data
> > after a flush.
> >
> > For example, if there is a region that is being resynchronized and we send
> > REQ_DISCARD that overlaps this region, there is no guarantee that data in
> > this region were zeroed.
> >
> > - kcopyd reads a few blocks for resynchronization
> > - REQ_DISCARD is sent to both mirror legs, both disks overwrites the area
> > with zeroes
> > - kcopyd writes those blocks to the other leg => the blocks are no longer
> > zero despite REQ_DISCARD being sent
>
> OK, thanks, but my point still stands: this is worthy of a separate
> patch (with the same type of backround you provided above).
>
> > 2. treat REQ_DISCARD as writes (i.e. synchronize it with region states,
> > wait until resynchronization finishes, etc.) --- it is possible to do it
> > this way to, but if we do it this way, we have to split REQ_DISCARD on
> > region boundaries (it is currently split only on target boundaries,
> > which is insufficient).
>
> I think discards should be treated as writes.

You can use this to fix the bug and treat discards as writes. But unlike
the previous patch, it causes discard splitting on region boundary.

Mikulas

---
drivers/md/dm.c | 5 ++++-
include/linux/device-mapper.h | 6 ++++++
2 files changed, 10 insertions(+), 1 deletion(-)

Index: linux-3.4.4-fast/drivers/md/dm.c
================================================== =================
--- linux-3.4.4-fast.orig/drivers/md/dm.c 2012-07-05 23:55:38.000000000 +0200
+++ linux-3.4.4-fast/drivers/md/dm.c 2012-07-05 23:56:41.000000000 +0200
@@ -1244,7 +1244,10 @@ static int __clone_and_map_discard(struc
if (!ti->num_discard_requests)
return -EOPNOTSUPP;

- len = min(ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
+ if (!ti->split_discard_requests)
+ len = min(ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
+ else
+ len = min(ci->sector_count, max_io_len(ci->sector, ti));

__issue_target_requests(ci, ti, ti->num_discard_requests, len);

Index: linux-3.4.4-fast/include/linux/device-mapper.h
================================================== =================
--- linux-3.4.4-fast.orig/include/linux/device-mapper.h 2012-07-05 23:53:47.000000000 +0200
+++ linux-3.4.4-fast/include/linux/device-mapper.h 2012-07-05 23:59:37.000000000 +0200
@@ -220,6 +220,12 @@ struct dm_target {
unsigned discards_supported:1;

/*
+ * Set if the target required discard request to be split
+ * on max_io_len boundary.
+ */
+ unsigned split_discard_requests:1;
+
+ /*
* Set if this target does not return zeroes on discarded blocks.
*/
unsigned discard_zeroes_data_unsupported:1;
---
drivers/md/dm-raid1.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-3.4.4-fast/drivers/md/dm-raid1.c
================================================== =================
--- linux-3.4.4-fast.orig/drivers/md/dm-raid1.c 2012-07-06 00:00:12.000000000 +0200
+++ linux-3.4.4-fast/drivers/md/dm-raid1.c 2012-07-06 00:01:54.000000000 +0200
@@ -677,8 +677,7 @@ static void do_writes(struct mirror_set
bio_list_init(&requeue);

while ((bio = bio_list_pop(writes))) {
- if ((bio->bi_rw & REQ_FLUSH) ||
- (bio->bi_rw & REQ_DISCARD)) {
+ if (bio->bi_rw & REQ_FLUSH) {
bio_list_add(&sync, bio);
continue;
}
@@ -1091,6 +1090,7 @@ static int mirror_ctr(struct dm_target *

ti->num_flush_requests = 1;
ti->num_discard_requests = 1;
+ ti->split_discard_requests = 1;

ms->kmirrord_wq = alloc_workqueue("kmirrord",
WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 10:53 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org