* bidi bsg is non-blocking @ 2007-05-07 15:21 Daniel.E.Messinger 2007-05-08 7:23 ` FUJITA Tomonori 0 siblings, 1 reply; 7+ messages in thread From: Daniel.E.Messinger @ 2007-05-07 15:21 UTC (permalink / raw) To: tomof; +Cc: linux-scsi Greetings to all, I'm attempting to use the bidi variant of bsg to talk to an OSD target device. I've run into an undesirable situation. My application has a free-running receive loop (doing a read() on the bsg device) waiting for responses to commands sent to bsg by another thread. The problem is that the receive thread is getting ENODATA from the read() when there are no commands pending. I have NOT set non-blocking. Note that the old sg driver was quite willing to block until there was a response available. I find this scenario greatly preferable. Could bsg be fixed so that read() blocks when there is nothing to return? Dan Messinger Seagate Technology Research ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-07 15:21 bidi bsg is non-blocking Daniel.E.Messinger @ 2007-05-08 7:23 ` FUJITA Tomonori 2007-05-08 7:31 ` FUJITA Tomonori 0 siblings, 1 reply; 7+ messages in thread From: FUJITA Tomonori @ 2007-05-08 7:23 UTC (permalink / raw) To: Daniel.E.Messinger; +Cc: jens.axboe, tomof, linux-scsi From: Daniel.E.Messinger@seagate.com Subject: bidi bsg is non-blocking Date: Mon, 7 May 2007 10:21:18 -0500 > I'm attempting to use the bidi variant of bsg to talk to an OSD target > device. I've run into an undesirable situation. > > My application has a free-running receive loop (doing a read() on the bsg > device) waiting for responses to commands sent to bsg by another thread. > The problem is that the receive thread is getting ENODATA from the read() > when there are no commands pending. I have NOT set non-blocking. > > Note that the old sg driver was quite willing to block until there was a > response available. I find this scenario greatly preferable. > > Could bsg be fixed so that read() blocks when there is nothing to return? I think that this is a bug. This patch is against the bsg branch in the Jens' git tree. I guess that it would be nice to do more cleanups on these parts. >From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001 From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Date: Tue, 8 May 2007 15:57:43 +0900 Subject: [PATCH] fix read ENODATA bug This patch fixes a bug that read() gives ENODATA even with a blocking file descriptor when there are no commands pending. This also includes some cleanups. Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> --- block/bsg.c | 85 ++++++++++++++-------------------------------------------- 1 files changed, 21 insertions(+), 64 deletions(-) diff --git a/block/bsg.c b/block/bsg.c index a333c93..7fcc77a 100644 --- a/block/bsg.c +++ b/block/bsg.c @@ -115,9 +115,9 @@ static void bsg_free_command(struct bsg_ wake_up(&bd->wq_free); } -static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd) +static struct bsg_command *bsg_alloc_command(struct bsg_device *bd) { - struct bsg_command *bc = NULL; + struct bsg_command *bc = ERR_PTR(-EINVAL); spin_lock_irq(&bd->lock); @@ -131,6 +131,7 @@ static struct bsg_command *__bsg_alloc_c if (unlikely(!bc)) { spin_lock_irq(&bd->lock); bd->queued_cmds--; + bc = ERR_PTR(-ENOMEM); goto out; } @@ -198,30 +199,6 @@ unlock: return ret; } -/* - * get a new free command, blocking if needed and specified - */ -static struct bsg_command *bsg_get_command(struct bsg_device *bd) -{ - struct bsg_command *bc; - int ret; - - do { - bc = __bsg_alloc_command(bd); - if (bc) - break; - - ret = bsg_io_schedule(bd, TASK_INTERRUPTIBLE); - if (ret) { - bc = ERR_PTR(ret); - break; - } - - } while (1); - - return bc; -} - static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq, struct sg_io_v4 *hdr, int has_write_perm) { @@ -397,7 +374,7 @@ static inline struct bsg_command *bsg_ne /* * Get a finished command from the done list */ -static struct bsg_command *__bsg_get_done_cmd(struct bsg_device *bd, int state) +static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd) { struct bsg_command *bc; int ret; @@ -407,11 +384,17 @@ static struct bsg_command *__bsg_get_don if (bc) break; - ret = bsg_io_schedule(bd, state); - if (ret) { - bc = ERR_PTR(ret); + if (!test_bit(BSG_F_BLOCK, &bd->flags)) { + bc = ERR_PTR(-EAGAIN); break; } + + ret = wait_event_interruptible(bd->wq_done, bd->done_cmds); + if (ret) { + bc = ERR_PTR(-ERESTARTSYS); + break; + } else + continue; } while (1); dprintk("%s: returning done %p\n", bd->name, bc); @@ -419,18 +402,6 @@ static struct bsg_command *__bsg_get_don return bc; } -static struct bsg_command * -bsg_get_done_cmd(struct bsg_device *bd, const struct iovec *iov) -{ - return __bsg_get_done_cmd(bd, TASK_INTERRUPTIBLE); -} - -static struct bsg_command * -bsg_get_done_cmd_nosignals(struct bsg_device *bd) -{ - return __bsg_get_done_cmd(bd, TASK_UNINTERRUPTIBLE); -} - static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr, struct bio *bio) { @@ -492,22 +463,13 @@ static int bsg_complete_all_commands(str } while (ret != -ENODATA); /* - * discard done commands + * discard done commands. */ ret = 0; do { - bc = bsg_get_done_cmd_nosignals(bd); - - /* - * we _must_ complete before restarting, because - * bsg_release can't handle this failing. - */ - if (PTR_ERR(bc) == -ERESTARTSYS) - continue; - if (IS_ERR(bc)) { - ret = PTR_ERR(bc); + bc = bsg_get_done_cmd(bd); + if (IS_ERR(bc)) break; - } tret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio); if (!ret) @@ -519,11 +481,9 @@ static int bsg_complete_all_commands(str return ret; } -typedef struct bsg_command *(*bsg_command_callback)(struct bsg_device *bd, const struct iovec *iov); - static ssize_t -__bsg_read(char __user *buf, size_t count, bsg_command_callback get_bc, - struct bsg_device *bd, const struct iovec *iov, ssize_t *bytes_read) +__bsg_read(char __user *buf, size_t count, struct bsg_device *bd, + const struct iovec *iov, ssize_t *bytes_read) { struct bsg_command *bc; int nr_commands, ret; @@ -534,7 +494,7 @@ __bsg_read(char __user *buf, size_t coun ret = 0; nr_commands = count / sizeof(struct sg_io_v4); while (nr_commands) { - bc = get_bc(bd, iov); + bc = bsg_get_done_cmd(bd); if (IS_ERR(bc)) { ret = PTR_ERR(bc); break; @@ -598,8 +558,7 @@ bsg_read(struct file *file, char __user bsg_set_block(bd, file); bytes_read = 0; - ret = __bsg_read(buf, count, bsg_get_done_cmd, - bd, NULL, &bytes_read); + ret = __bsg_read(buf, count, bd, NULL, &bytes_read); *ppos = bytes_read; if (!bytes_read || (bytes_read && err_block_err(ret))) @@ -625,9 +584,7 @@ static ssize_t __bsg_write(struct bsg_de while (nr_commands) { request_queue_t *q = bd->queue; - bc = bsg_get_command(bd); - if (!bc) - break; + bc = bsg_alloc_command(bd); if (IS_ERR(bc)) { ret = PTR_ERR(bc); bc = NULL; -- 1.4.3.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-08 7:23 ` FUJITA Tomonori @ 2007-05-08 7:31 ` FUJITA Tomonori 2007-05-08 12:21 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: FUJITA Tomonori @ 2007-05-08 7:31 UTC (permalink / raw) To: Daniel.E.Messinger; +Cc: jens.axboe, tomof, linux-scsi From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Subject: Re: bidi bsg is non-blocking Date: Tue, 08 May 2007 16:23:26 +0900 > From: Daniel.E.Messinger@seagate.com > Subject: bidi bsg is non-blocking > Date: Mon, 7 May 2007 10:21:18 -0500 > > > I'm attempting to use the bidi variant of bsg to talk to an OSD target > > device. I've run into an undesirable situation. > > > > My application has a free-running receive loop (doing a read() on the bsg > > device) waiting for responses to commands sent to bsg by another thread. > > The problem is that the receive thread is getting ENODATA from the read() > > when there are no commands pending. I have NOT set non-blocking. > > > > Note that the old sg driver was quite willing to block until there was a > > response available. I find this scenario greatly preferable. > > > > Could bsg be fixed so that read() blocks when there is nothing to return? > > I think that this is a bug. > > This patch is against the bsg branch in the Jens' git tree. > > I guess that it would be nice to do more cleanups on these parts. > > > From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001 > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > Date: Tue, 8 May 2007 15:57:43 +0900 > Subject: [PATCH] fix read ENODATA bug > > This patch fixes a bug that read() gives ENODATA even with a blocking > file descriptor when there are no commands pending. > > This also includes some cleanups. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Oops, I sent a wrong patch. This is a right one. --- >From 115a65feaadae0c100b5366670eb729e9e6fd243 Mon Sep 17 00:00:00 2001 From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Date: Tue, 8 May 2007 16:28:36 +0900 Subject: [PATCH] fix read ENODATA bug This patch fixes a bug that read() gives ENODATA even with a blocking file descriptor when there are no commands pending. This also includes some cleanups. Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> --- block/bsg.c | 89 +++++++++++++++++----------------------------------------- 1 files changed, 26 insertions(+), 63 deletions(-) diff --git a/block/bsg.c b/block/bsg.c index a333c93..a5a71fc 100644 --- a/block/bsg.c +++ b/block/bsg.c @@ -115,9 +115,9 @@ static void bsg_free_command(struct bsg_ wake_up(&bd->wq_free); } -static struct bsg_command *__bsg_alloc_command(struct bsg_device *bd) +static struct bsg_command *bsg_alloc_command(struct bsg_device *bd) { - struct bsg_command *bc = NULL; + struct bsg_command *bc = ERR_PTR(-EINVAL); spin_lock_irq(&bd->lock); @@ -131,6 +131,7 @@ static struct bsg_command *__bsg_alloc_c if (unlikely(!bc)) { spin_lock_irq(&bd->lock); bd->queued_cmds--; + bc = ERR_PTR(-ENOMEM); goto out; } @@ -198,30 +199,6 @@ unlock: return ret; } -/* - * get a new free command, blocking if needed and specified - */ -static struct bsg_command *bsg_get_command(struct bsg_device *bd) -{ - struct bsg_command *bc; - int ret; - - do { - bc = __bsg_alloc_command(bd); - if (bc) - break; - - ret = bsg_io_schedule(bd, TASK_INTERRUPTIBLE); - if (ret) { - bc = ERR_PTR(ret); - break; - } - - } while (1); - - return bc; -} - static int blk_fill_sgv4_hdr_rq(request_queue_t *q, struct request *rq, struct sg_io_v4 *hdr, int has_write_perm) { @@ -397,7 +374,7 @@ static inline struct bsg_command *bsg_ne /* * Get a finished command from the done list */ -static struct bsg_command *__bsg_get_done_cmd(struct bsg_device *bd, int state) +static struct bsg_command *bsg_get_done_cmd(struct bsg_device *bd) { struct bsg_command *bc; int ret; @@ -407,11 +384,17 @@ static struct bsg_command *__bsg_get_don if (bc) break; - ret = bsg_io_schedule(bd, state); - if (ret) { - bc = ERR_PTR(ret); + if (!test_bit(BSG_F_BLOCK, &bd->flags)) { + bc = ERR_PTR(-EAGAIN); break; } + + ret = wait_event_interruptible(bd->wq_done, bd->done_cmds); + if (ret) { + bc = ERR_PTR(-ERESTARTSYS); + break; + } else + continue; } while (1); dprintk("%s: returning done %p\n", bd->name, bc); @@ -419,18 +402,6 @@ static struct bsg_command *__bsg_get_don return bc; } -static struct bsg_command * -bsg_get_done_cmd(struct bsg_device *bd, const struct iovec *iov) -{ - return __bsg_get_done_cmd(bd, TASK_INTERRUPTIBLE); -} - -static struct bsg_command * -bsg_get_done_cmd_nosignals(struct bsg_device *bd) -{ - return __bsg_get_done_cmd(bd, TASK_UNINTERRUPTIBLE); -} - static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr, struct bio *bio) { @@ -492,23 +463,20 @@ static int bsg_complete_all_commands(str } while (ret != -ENODATA); /* - * discard done commands + * discard done commands. */ ret = 0; do { - bc = bsg_get_done_cmd_nosignals(bd); - - /* - * we _must_ complete before restarting, because - * bsg_release can't handle this failing. - */ - if (PTR_ERR(bc) == -ERESTARTSYS) - continue; - if (IS_ERR(bc)) { - ret = PTR_ERR(bc); + spin_lock_irq(&bd->lock); + if (!bd->queued_cmds) { + spin_unlock_irq(&bd->lock); break; } + bc = bsg_get_done_cmd(bd); + if (IS_ERR(bc)) + break; + tret = blk_complete_sgv4_hdr_rq(bc->rq, &bc->hdr, bc->bio); if (!ret) ret = tret; @@ -519,11 +487,9 @@ static int bsg_complete_all_commands(str return ret; } -typedef struct bsg_command *(*bsg_command_callback)(struct bsg_device *bd, const struct iovec *iov); - static ssize_t -__bsg_read(char __user *buf, size_t count, bsg_command_callback get_bc, - struct bsg_device *bd, const struct iovec *iov, ssize_t *bytes_read) +__bsg_read(char __user *buf, size_t count, struct bsg_device *bd, + const struct iovec *iov, ssize_t *bytes_read) { struct bsg_command *bc; int nr_commands, ret; @@ -534,7 +500,7 @@ __bsg_read(char __user *buf, size_t coun ret = 0; nr_commands = count / sizeof(struct sg_io_v4); while (nr_commands) { - bc = get_bc(bd, iov); + bc = bsg_get_done_cmd(bd); if (IS_ERR(bc)) { ret = PTR_ERR(bc); break; @@ -598,8 +564,7 @@ bsg_read(struct file *file, char __user bsg_set_block(bd, file); bytes_read = 0; - ret = __bsg_read(buf, count, bsg_get_done_cmd, - bd, NULL, &bytes_read); + ret = __bsg_read(buf, count, bd, NULL, &bytes_read); *ppos = bytes_read; if (!bytes_read || (bytes_read && err_block_err(ret))) @@ -625,9 +590,7 @@ static ssize_t __bsg_write(struct bsg_de while (nr_commands) { request_queue_t *q = bd->queue; - bc = bsg_get_command(bd); - if (!bc) - break; + bc = bsg_alloc_command(bd); if (IS_ERR(bc)) { ret = PTR_ERR(bc); bc = NULL; -- 1.4.3.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-08 7:31 ` FUJITA Tomonori @ 2007-05-08 12:21 ` Jens Axboe 2007-05-08 12:56 ` FUJITA Tomonori 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2007-05-08 12:21 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: Daniel.E.Messinger, tomof, linux-scsi On Tue, May 08 2007, FUJITA Tomonori wrote: > > From: Daniel.E.Messinger@seagate.com > > Subject: bidi bsg is non-blocking > > Date: Mon, 7 May 2007 10:21:18 -0500 > > > > > I'm attempting to use the bidi variant of bsg to talk to an OSD target > > > device. I've run into an undesirable situation. > > > > > > My application has a free-running receive loop (doing a read() on the bsg > > > device) waiting for responses to commands sent to bsg by another thread. > > > The problem is that the receive thread is getting ENODATA from the read() > > > when there are no commands pending. I have NOT set non-blocking. > > > > > > Note that the old sg driver was quite willing to block until there was a > > > response available. I find this scenario greatly preferable. > > > > > > Could bsg be fixed so that read() blocks when there is nothing to return? > > > > I think that this is a bug. > > > > This patch is against the bsg branch in the Jens' git tree. > > > > I guess that it would be nice to do more cleanups on these parts. > > > > > > From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001 > > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > > Date: Tue, 8 May 2007 15:57:43 +0900 > > Subject: [PATCH] fix read ENODATA bug > > > > This patch fixes a bug that read() gives ENODATA even with a blocking > > file descriptor when there are no commands pending. > > > > This also includes some cleanups. > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > > Oops, I sent a wrong patch. This is a right one. Patch looks good, I agree this is the way that bsg should act for a blocking read on an "empty" queue. > + ret = wait_event_interruptible(bd->wq_done, bd->done_cmds); > + if (ret) { > + bc = ERR_PTR(-ERESTARTSYS); > + break; > + } else > + continue; > } while (1); The else clause is pointless. Apart from that, it looks sounds. Can you resend? -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-08 12:21 ` Jens Axboe @ 2007-05-08 12:56 ` FUJITA Tomonori 2007-05-08 13:00 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: FUJITA Tomonori @ 2007-05-08 12:56 UTC (permalink / raw) To: jens.axboe; +Cc: fujita.tomonori, Daniel.E.Messinger, tomof, linux-scsi From: Jens Axboe <jens.axboe@oracle.com> Subject: Re: bidi bsg is non-blocking Date: Tue, 8 May 2007 14:21:34 +0200 > On Tue, May 08 2007, FUJITA Tomonori wrote: > > > From: Daniel.E.Messinger@seagate.com > > > Subject: bidi bsg is non-blocking > > > Date: Mon, 7 May 2007 10:21:18 -0500 > > > > > > > I'm attempting to use the bidi variant of bsg to talk to an OSD target > > > > device. I've run into an undesirable situation. > > > > > > > > My application has a free-running receive loop (doing a read() on the bsg > > > > device) waiting for responses to commands sent to bsg by another thread. > > > > The problem is that the receive thread is getting ENODATA from the read() > > > > when there are no commands pending. I have NOT set non-blocking. > > > > > > > > Note that the old sg driver was quite willing to block until there was a > > > > response available. I find this scenario greatly preferable. > > > > > > > > Could bsg be fixed so that read() blocks when there is nothing to return? > > > > > > I think that this is a bug. > > > > > > This patch is against the bsg branch in the Jens' git tree. > > > > > > I guess that it would be nice to do more cleanups on these parts. > > > > > > > > > From 52a9fcf0af72f212ddd93918b7f9f0f0e706c215 Mon Sep 17 00:00:00 2001 > > > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > > > Date: Tue, 8 May 2007 15:57:43 +0900 > > > Subject: [PATCH] fix read ENODATA bug > > > > > > This patch fixes a bug that read() gives ENODATA even with a blocking > > > file descriptor when there are no commands pending. > > > > > > This also includes some cleanups. > > > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > > > > Oops, I sent a wrong patch. This is a right one. > > Patch looks good, I agree this is the way that bsg should act for a > blocking read on an "empty" queue. > > > + ret = wait_event_interruptible(bd->wq_done, bd->done_cmds); > > + if (ret) { > > + bc = ERR_PTR(-ERESTARTSYS); > > + break; > > + } else > > + continue; > > } while (1); > > The else clause is pointless. Oops. > Apart from that, it looks sounds. Can you resend? Sure, I'll resend an updated version shortly. BTW, any comments on the bidi patch for the block layer? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-08 12:56 ` FUJITA Tomonori @ 2007-05-08 13:00 ` Jens Axboe 2007-05-08 13:30 ` FUJITA Tomonori 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2007-05-08 13:00 UTC (permalink / raw) To: FUJITA Tomonori; +Cc: Daniel.E.Messinger, tomof, linux-scsi On Tue, May 08 2007, FUJITA Tomonori wrote: > BTW, any comments on the bidi patch for the block layer? Yeah it looks much better. I agree with your notion that bidi should be block layer agnostic at least for now (until a general use comes up), it's just a way to transport these commands. And I do still prefer ->next_rq as the pointer name for partly those reasons. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bidi bsg is non-blocking 2007-05-08 13:00 ` Jens Axboe @ 2007-05-08 13:30 ` FUJITA Tomonori 0 siblings, 0 replies; 7+ messages in thread From: FUJITA Tomonori @ 2007-05-08 13:30 UTC (permalink / raw) To: jens.axboe; +Cc: fujita.tomonori, Daniel.E.Messinger, tomof, linux-scsi From: Jens Axboe <jens.axboe@oracle.com> Subject: Re: bidi bsg is non-blocking Date: Tue, 8 May 2007 15:00:32 +0200 > On Tue, May 08 2007, FUJITA Tomonori wrote: > > BTW, any comments on the bidi patch for the block layer? > > Yeah it looks much better. I agree with your notion that bidi should be > block layer agnostic at least for now (until a general use comes up), > it's just a way to transport these commands. And I do still prefer > ->next_rq as the pointer name for partly those reasons. Thanks for the comments. Seems that the block layer part is done. And my patch uses ->next_rq pointer name: http://marc.info/?l=linux-scsi&m=117835587615642&w=2 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2007-05-08 13:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-05-07 15:21 bidi bsg is non-blocking Daniel.E.Messinger 2007-05-08 7:23 ` FUJITA Tomonori 2007-05-08 7:31 ` FUJITA Tomonori 2007-05-08 12:21 ` Jens Axboe 2007-05-08 12:56 ` FUJITA Tomonori 2007-05-08 13:00 ` Jens Axboe 2007-05-08 13:30 ` FUJITA Tomonori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox