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 03-19-2012, 02:36 PM
Hannes Reinecke
 
Default dm-mpath: Track invalid map_context

On 03/19/2012 04:20 PM, Alasdair G Kergon wrote:
> On Mon, Mar 19, 2012 at 04:15:28PM +0100, Hannes Reinecke wrote:
>> - if (r < 0 || r == DM_MAPIO_REQUEUE)
>> + if (r < 0 || r == DM_MAPIO_REQUEUE) {
>> mempool_free(mpio, m->mpio_pool);
>> + map_context->ptr = NULL;
>> + }
>
> What about the other places that do mempool_free() ?
> Should they clear it too?
>
Hmm. Probably. It's not strictly speaking required as the other
places will never re-use the context pointer.
But for consistencies sake you are correct.
Will be updating the patch.

> Is it better to swap the statement order - clear it *before*
> freeing i
>
Doubt that should be required.
I would hope that the map_context pointer is protected by
appropriate locks.

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-19-2012, 02:39 PM
Hannes Reinecke
 
Default dm-mpath: Track invalid map_context

The map_context pointer should always be set. However, we
have reports that upon requeing it is not set correctly.
So add a BUG_ON() statement and clear the pointer to
track the issue properly.

Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Acked-by: Dave Wysochanski <dwysocha@redhat.com>
---
drivers/md/dm-mpath.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 801d92d..2ed316e 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -441,11 +441,13 @@ static void dispatch_queued_ios(struct multipath *m)
r = map_io(m, clone, mpio, 1);
if (r < 0) {
mempool_free(mpio, m->mpio_pool);
+ info->ptr = NULL;
dm_kill_unmapped_request(clone, r);
} else if (r == DM_MAPIO_REMAPPED)
dm_dispatch_request(clone);
else if (r == DM_MAPIO_REQUEUE) {
mempool_free(mpio, m->mpio_pool);
+ info->ptr = NULL;
dm_requeue_unmapped_request(clone);
}
}
@@ -920,8 +922,10 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
map_context->ptr = mpio;
clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
r = map_io(m, clone, mpio, 0);
- if (r < 0 || r == DM_MAPIO_REQUEUE)
+ if (r < 0 || r == DM_MAPIO_REQUEUE) {
mempool_free(mpio, m->mpio_pool);
+ map_context->ptr = NULL;
+ }

return r;
}
@@ -1261,6 +1265,8 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
struct path_selector *ps;
int r;

+ BUG_ON(!mpio);
+
r = do_end_io(m, clone, error, mpio);
if (pgpath) {
ps = &pgpath->pg->ps;
@@ -1268,6 +1274,7 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
}
mempool_free(mpio, m->mpio_pool);
+ map_context->ptr = NULL;

return r;
}
--
1.6.0.2

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-21-2012, 03:16 AM
"Jun'ichi Nomura"
 
Default dm-mpath: Track invalid map_context

Hi Hannes,

On 03/20/12 00:39, Hannes Reinecke wrote:
> The map_context pointer should always be set. However, we
> have reports that upon requeing it is not set correctly.
> So add a BUG_ON() statement and clear the pointer to
> track the issue properly.
>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Acked-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
> drivers/md/dm-mpath.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 801d92d..2ed316e 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -441,11 +441,13 @@ static void dispatch_queued_ios(struct multipath *m)
> r = map_io(m, clone, mpio, 1);
> if (r < 0) {
> mempool_free(mpio, m->mpio_pool);
> + info->ptr = NULL;
> dm_kill_unmapped_request(clone, r);
> } else if (r == DM_MAPIO_REMAPPED)
> dm_dispatch_request(clone);
> else if (r == DM_MAPIO_REQUEUE) {
> mempool_free(mpio, m->mpio_pool);
> + info->ptr = NULL;
> dm_requeue_unmapped_request(clone);
> }
> }

If we want to ensure info->ptr is either valid or NULL,
how about introducing a function to do it like this?

---
Jun'ichi Nomura, NEC Corporation


diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 801d92d..c6c33b0 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -226,6 +226,26 @@ static void free_multipath(struct multipath *m)
kfree(m);
}

+static int set_mapinfo(struct multipath *m, union map_info *info)
+{
+ struct dm_mpath_io *mpio;
+
+ mpio = mempool_alloc(m->mpio_pool, GFP_ATOMIC);
+ if (!mpio)
+ return -ENOMEM;
+
+ memset(mpio, 0, sizeof(*mpio));
+ info->ptr = mpio;
+
+ return 0;
+}
+
+static void clear_mapinfo(struct multipath *m, union map_info *info)
+{
+ struct dm_mpath_io *mpio = info->ptr;
+ info->ptr = NULL;
+ mempool_free(mpio, m->mpio_pool);
+}

/*-----------------------------------------------
* Path selection
@@ -341,13 +361,14 @@ static int __must_push_back(struct multipath *m)
}

static int map_io(struct multipath *m, struct request *clone,
- struct dm_mpath_io *mpio, unsigned was_queued)
+ union map_info *map_context, unsigned was_queued)
{
int r = DM_MAPIO_REMAPPED;
size_t nr_bytes = blk_rq_bytes(clone);
unsigned long flags;
struct pgpath *pgpath;
struct block_device *bdev;
+ struct dm_mpath_io *mpio = map_context->ptr;

spin_lock_irqsave(&m->lock, flags);

@@ -423,7 +444,6 @@ static void dispatch_queued_ios(struct multipath *m)
{
int r;
unsigned long flags;
- struct dm_mpath_io *mpio;
union map_info *info;
struct request *clone, *n;
LIST_HEAD(cl);
@@ -436,16 +456,15 @@ static void dispatch_queued_ios(struct multipath *m)
list_del_init(&clone->queuelist);

info = dm_get_rq_mapinfo(clone);
- mpio = info->ptr;

- r = map_io(m, clone, mpio, 1);
+ r = map_io(m, clone, info, 1);
if (r < 0) {
- mempool_free(mpio, m->mpio_pool);
+ clear_mapinfo(m, info);
dm_kill_unmapped_request(clone, r);
} else if (r == DM_MAPIO_REMAPPED)
dm_dispatch_request(clone);
else if (r == DM_MAPIO_REQUEUE) {
- mempool_free(mpio, m->mpio_pool);
+ clear_mapinfo(m, info);
dm_requeue_unmapped_request(clone);
}
}
@@ -908,20 +927,16 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
union map_info *map_context)
{
int r;
- struct dm_mpath_io *mpio;
struct multipath *m = (struct multipath *) ti->private;

- mpio = mempool_alloc(m->mpio_pool, GFP_ATOMIC);
- if (!mpio)
+ if (set_mapinfo(m, map_context) < 0)
/* ENOMEM, requeue */
return DM_MAPIO_REQUEUE;
- memset(mpio, 0, sizeof(*mpio));

- map_context->ptr = mpio;
clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
- r = map_io(m, clone, mpio, 0);
+ r = map_io(m, clone, map_context, 0);
if (r < 0 || r == DM_MAPIO_REQUEUE)
- mempool_free(mpio, m->mpio_pool);
+ clear_mapinfo(m, map_context);

return r;
}
@@ -1261,13 +1276,15 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
struct path_selector *ps;
int r;

+ BUG_ON(!mpio);
+
r = do_end_io(m, clone, error, mpio);
if (pgpath) {
ps = &pgpath->pg->ps;
if (ps->type->end_io)
ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
}
- mempool_free(mpio, m->mpio_pool);
+ clear_mapinfo(m, map_context);

return r;
}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-23-2012, 10:33 PM
Alasdair G Kergon
 
Default dm-mpath: Track invalid map_context

I've opted for this version.

http://people.redhat.com/agk/patches/linux/editing/dm-mpath-detect-invalid-map_context.patch

Alasdair

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-26-2012, 06:18 AM
Hannes Reinecke
 
Default dm-mpath: Track invalid map_context

On 03/24/2012 12:33 AM, Alasdair G Kergon wrote:
> I've opted for this version.
>
> http://people.redhat.com/agk/patches/linux/editing/dm-mpath-detect-invalid-map_context.patch
>
> Alasdair
>
Yeah. I'm used to my patches getting rewritten by others.
And me not getting any credit for it. Oh well.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

--
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 06:07 AM.

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