Re: [PATCH 1/9] QUEUE_FLAG_NOWAIT to indicate device supports nowait

On 08/09/2017 08:17 PM, Shaohua Li wrote:

On Wed, Aug 09, 2017 at 05:16:23PM -0500, Goldwyn Rodrigues wrote:

quoted

On 08/09/2017 03:21 PM, Shaohua Li wrote:

quoted

On Wed, Aug 09, 2017 at 10:35:39AM -0500, Goldwyn Rodrigues wrote:

quoted

On 08/09/2017 10:02 AM, Shaohua Li wrote:

quoted

On Wed, Aug 09, 2017 at 06:44:55AM -0500, Goldwyn Rodrigues wrote:

quoted

On 08/08/2017 03:32 PM, Shaohua Li wrote:

quoted

On Wed, Jul 26, 2017 at 06:57:58PM -0500, Goldwyn Rodrigues wrote:

quoted

From: Goldwyn Rodrigues <redacted>

Nowait is a feature of direct AIO, where users can request
to return immediately if the I/O is going to block. This translates
to REQ_NOWAIT in bio.bi_opf flags. While request based devices
don't wait, stacked devices such as md/dm will.

In order to explicitly mark stacked devices as supported, we
set the QUEUE_FLAG_NOWAIT in the queue_flags and return -EAGAIN
whenever the device would block.

probably you should route this patch to Jens first, DM/MD are different trees.

Yes, I have sent it to linux-block as well, and he has commented as well.

quoted

quoted

Signed-off-by: Goldwyn Rodrigues <redacted>
---
 block/blk-core.c       | 3 ++-
 include/linux/blkdev.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 970b9c9638c5..1c9a981d88e5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -2025,7 +2025,8 @@ generic_make_request_checks(struct bio *bio)
 	 * if queue is not a request based queue.
 	 */
 
-	if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q))
+	if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q) &&
+	    !blk_queue_supports_nowait(q))
 		goto not_supported;
 
 	part = bio->bi_bdev->bd_part;

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 25f6a0cb27d3..fae021ebec1b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -633,6 +633,7 @@ struct request_queue {
 #define QUEUE_FLAG_REGISTERED  29	/* queue has been registered to a disk */
 #define QUEUE_FLAG_SCSI_PASSTHROUGH 30	/* queue supports SCSI commands */
 #define QUEUE_FLAG_QUIESCED    31	/* queue has been quiesced */
+#define QUEUE_FLAG_NOWAIT      32	/* stack device driver supports REQ_NOWAIT */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\

@@ -732,6 +733,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 #define blk_queue_dax(q)	test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
 #define blk_queue_scsi_passthrough(q)	\
 	test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
+#define blk_queue_supports_nowait(q)	test_bit(QUEUE_FLAG_NOWAIT, &(q)->queue_flags)

Should this bit consider under layer disks? For example, one raid array disk
doesn't support NOWAIT, shouldn't we disable NOWAIT for the array?

Yes, it should. I will add a check before setting the flag. Thanks.
Request-based devices don't wait. So, they would not have this flag set.
It is only the bio-based, with the  make_request_fn hook which need this.

quoted

 
I have another generic question. If a bio is splitted into 2 bios, one bio
doesn't need to wait but the other need to wait. We will return -EAGAIN for the
second bio, so the whole bio will return -EAGAIN, but the first bio is already
dispatched to disk. Is this correct behavior?

No, from a multi-device point of view, this is inconsistent. I have
tried the request bio returns -EAGAIN before the split, but I shall
check again. Where do you see this happening?

No, this isn't multi-device specific, any driver can do it. Please see blk_queue_split.

In that case, the bio end_io function is chained and the bio of the
split will replicate the error to the parent (if not already set).

this doesn't answer my question. So if a bio returns -EAGAIN, part of the bio
probably already dispatched to disk (if the bio is splitted to 2 bios, one
returns -EAGAIN, the other one doesn't block and dispatch to disk), what will
application be going to do? I think this is different to other IO errors. FOr
other IO errors, application will handle the error, while we ask app to retry
the whole bio here and app doesn't know part of bio is already written to disk.

It is the same as for other I/O errors as well, such as EIO. You do not
know which bio of all submitted bio's returned the error EIO. The
application would and should consider the whole I/O as failed.

The user application does not know of bios, or how it is going to be
split in the underlying layers. It knows at the system call level. In
this case, the EAGAIN will be returned to the user for the whole I/O not
as a part of the I/O. It is up to application to try the I/O again with
or without RWF_NOWAIT set. In direct I/O, it is bubbled out using
dio->io_error. You can read about it at the patch header for the initial
patchset at [1].

Use case: It is for applications having two threads, a compute thread
and an I/O thread. It would try to push AIO as much as possible in the
compute thread using RWF_NOWAIT, and if it fails, would pass it on to
I/O thread which would perform without RWF_NOWAIT. End result if done
right is you save on context switches and all the
synchronization/messaging machinery to perform I/O.

[1] http://marc.info/?l=linux-block&m=149789003305876&w=2

Yes, I knew the concept, but I didn't see previous patches mentioned the
-EAGAIN actually should be taken as a real IO error. This means a lot to
applications and make the API hard to use. I'm wondering if we should disable
bio split for NOWAIT bio, which will make the -EAGAIN only mean 'try again'.

Don't take it as EAGAIN, but read it as EWOULDBLOCK. Why do you say the
API is hard to use? Do you have a case to back it up?

No, not splitting the bio does not make sense here. I do not see any
advantage in it, unless you can present a case otherwise.


-- 
Goldwyn