lkml.org 
[lkml]   [2003]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] 2.6.0 NBD driver: remove send/recieve race for request
    Lou Langholtz wrote:

    > >--- linux-2.6.0-test2-mm4-PRISTINE/drivers/block/nbd.c Sun Jul 27 12:58:51 2003
    > >+++ linux-2.6.0-test2-mm4/drivers/block/nbd.c Thu Aug 7 18:02:23 2003
    > >@@ -416,11 +416,19 @@ void nbd_clear_que(struct nbd_device *lo
    > > BUG_ON(lo->magic != LO_MAGIC);
    > > #endif
    > >
    > >+retry:
    > > do {
    > > req = NULL;
    > > spin_lock(&lo->queue_lock);
    > > if (!list_empty(&lo->queue_head)) {
    > > req = list_entry(lo->queue_head.next, struct request, queuelist);
    > >+ if (req->ref_count > 1) { /* still in xmit */
    > >+ spin_unlock(&lo->queue_lock);
    > >+ printk(KERN_DEBUG "%s: request %p: still in use (%d), waiting...\n",
    > >+ lo->disk->disk_name, req, req->ref_count);
    > >+ schedule_timeout(HZ); /* wait a second */
    > >

    > Isn't there something more deterministic than just waiting a second and
    > hoping things clear up that you can use here?

    It's not exactly "hoping" something will happen...we're waiting until
    do_nbd_request decrements the ref_count, which is completely
    deterministic, but just not guaranteed to have already happened when
    nbd_clear_que() starts.

    The use of the ref_count closes a race window (the one that I pointed
    out in my response to Lou's original patch a few days ago). It protects
    us in the following case(s):

    1) do_nbd_request begins -- sock and file are valid
    2) do_nbd_request queues the request and then calls nbd_send_req to send
    the request out on the network
    3) on another CPU, or after preempt, nbd-client gets a signal or an
    "nbd-client -d" is called, which results in sock and file being set to
    NULL, and the queue begins to be cleared, and requests that were on the
    queue get freed (before do_nbd_request has finished accessing req for
    the last time)


    > How about not clearing the
    > queue unless lo->sock is NULL and using whatever lock it is now that's
    > protecting lo->sock. That way the queue clearing race can be eliminated too.

    That makes sense. I hadn't done this (for compatibility reasons) since
    "nbd-client -d" (disconnect) calls NBD_CLEAR_QUE before disconnecting
    the socket. But I think we can just make NBD_CLEAR_QUE silently fail
    (return 0) if lo->sock is set and basically turn that first
    NBD_CLEAR_QUE into a noop, and let the nbd_clear_que() call at the end
    of NBD_CLEAR_SOCK handle it (properly). Note that the race condition
    above still applies, even with this lo->sock check in place. But this
    check does eliminate the race in "nbd-client -d" where, e.g., requests
    are being queued up faster than nbd_clear_que can dequeue them,
    potentially making nbd_clear_que loop forever.

    As a side note, NBD_CLEAR_QUE is actually now completely superfluous,
    since NBD_DO_IT and NBD_CLEAR_SOCK (which always get called in
    conjunction with NBD_CLEAR_QUE) contain the nbd_clear_que() call
    themselves. We could just make NBD_CLEAR_QUE a noop in all cases, but I
    guess we'd risk breaking some nbd client tool that's out there...

    I'm testing the updated patch now, I'll send it out in a little while...

    Thanks,
    Paul
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 13:47    [W:0.043 / U:64.480 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site