[dm-devel] dm-io: Prevent the danging point of the sync io callback function

[huangminfei]

yes, this patch more prefer than I summitted. 


--------------------
黄敏飞 平台研发部
上海优刻得信息科技有限公司
Ucloud--做中国最专业的IAAS运营商
QQ:    805852007
地址：上海市杨浦区国定东路200号3号楼2楼


在2014年06月28 03时29分，"Mike Snitzer"<snitzer at redhat.com>写道：

On Fri, Jun 27 2014 at  2:44pm -0400,
Mikulas Patocka <mpatocka at redhat.com> wrote:

>
>
> On Fri, 27 Jun 2014, Joe Thornber wrote:
>
> > On Fri, Jun 27, 2014 at 12:01:30PM +0800, Minfei Huang wrote:
> > > The io address in callback function will become the danging point,
> > > cause by the thread of sync io wakes up by other threads
> > > and return to relieve the io address,
> >
> > Yes, well found.  I prefer the following fix however.
> >
> > - Joe
>
> It seems ok.
>
> The patch is too big, I think the only change that needs to be done to fix
> the bug is to replace "struct task_struct *sleeper;" with "struct
> completion *completion;", replace "if (io->sleeper)
> wake_up_process(io->sleeper);" with "if (io->completion)
> complete(io->completion);" and declare the completion in sync_io() and
> wait on it instead of "while (1)" loop there.

Here is the minimalist fix you suggested (I agree that splitting out a
minimalist fix is useful):

drivers/md/dm-io.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
index 2a20986..e60c2ea 100644
--- a/drivers/md/dm-io.c
+++ b/drivers/md/dm-io.c
@@ -10,6 +10,7 @@
#include <linux/device-mapper.h>

#include <linux/bio.h>
+#include <linux/completion.h>
#include <linux/mempool.h>
#include <linux/module.h>
#include <linux/sched.h>
@@ -32,7 +33,7 @@ struct dm_io_client {
struct io {
    unsigned long error_bits;
    atomic_t count;
-    struct task_struct *sleeper;
+    struct completion *wait;
    struct dm_io_client *client;
    io_notify_fn callback;
    void *context;
@@ -121,8 +122,8 @@ static void dec_count(struct io *io, unsigned int region, int error)
            invalidate_kernel_vmap_range(io->vma_invalidate_address,
                             io->vma_invalidate_size);

-        if (io->sleeper)
-            wake_up_process(io->sleeper);
+        if (io->wait)
+            complete(io->wait);

        else {
            unsigned long r = io->error_bits;
@@ -385,6 +386,7 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions,
     */
    volatile char io_[sizeof(struct io) + __alignof__(struct io) - 1];
    struct io *io = (struct io *)PTR_ALIGN(&io_, __alignof__(struct io));
+    DECLARE_COMPLETION_ONSTACK(wait);

    if (num_regions > 1 && (rw & RW_MASK) != WRITE) {
        WARN_ON(1);
@@ -393,7 +395,7 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions,

    io->error_bits = 0;
    atomic_set(&io->count, 1); /* see dispatch_io() */
-    io->sleeper = current;
+    io->wait = &wait;
    io->client = client;

    io->vma_invalidate_address = dp->vma_invalidate_address;
@@ -401,15 +403,7 @@ static int sync_io(struct dm_io_client *client, unsigned int num_regions,

    dispatch_io(rw, num_regions, where, dp, io, 1);

-    while (1) {
-        set_current_state(TASK_UNINTERRUPTIBLE);
-
-        if (!atomic_read(&io->count))
-            break;
-
-        io_schedule();
-    }
-    set_current_state(TASK_RUNNING);
+    wait_for_completion_io(&wait);

    if (error_bits)
        *error_bits = io->error_bits;
@@ -432,7 +426,7 @@ static int async_io(struct dm_io_client *client, unsigned int num_regions,
    io = mempool_alloc(client->pool, GFP_NOIO);
    io->error_bits = 0;
    atomic_set(&io->count, 1); /* see dispatch_io() */
-    io->sleeper = NULL;
+    io->wait = NULL;
    io->client = client;
    io->callback = fn;
    io->context = context;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20140628/1ec67daf/attachment.htm>