| Message ID | 20170228233610.25456-2-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 28, 2017 at 05:36:03PM -0600, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > This flag informs kernel to bail out if an AIO request will block > for reasons such as file allocations, or a writeback triggered, > or would block while allocating requests while performing > direct I/O. > > IOCB_FLAG_NOWAIT is translated to IOCB_NOWAIT for > iocb->ki_flags. Given that we aren't validating aio_flags in older kernels we can't just add this flag as it will be a no-op in older kernels. I think we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that properly validate all reserved fields or flags first. Once we do that I'd really prefer to use the same flags values as preadv2/pwritev2 so that we'll only need one set of flags over sync/async read/write ops.
On Wed, Mar 01, 2017 at 07:36:48AM -0800, Christoph Hellwig wrote: > Given that we aren't validating aio_flags in older kernels we can't > just add this flag as it will be a no-op in older kernels. I think > we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that > properly validate all reserved fields or flags first. > > Once we do that I'd really prefer to use the same flags values > as preadv2/pwritev2 so that we'll only need one set of flags over > sync/async read/write ops. I just took another look and we do verify that aio_reserved1/aio_reserved2 must be zero. So I think we can just stick RWF_* into aio_reserved1 and fix that problem that way.
On 03/01/2017 09:56 AM, Christoph Hellwig wrote: > On Wed, Mar 01, 2017 at 07:36:48AM -0800, Christoph Hellwig wrote: >> Given that we aren't validating aio_flags in older kernels we can't >> just add this flag as it will be a no-op in older kernels. I think >> we will have to add IOCB_CMD_PREADV2/IOCB_CMD_WRITEV2 opcodes that >> properly validate all reserved fields or flags first. >> >> Once we do that I'd really prefer to use the same flags values >> as preadv2/pwritev2 so that we'll only need one set of flags over >> sync/async read/write ops. > > I just took another look and we do verify that > aio_reserved1/aio_reserved2 must be zero. So I think we can just > stick RWF_* into aio_reserved1 and fix that problem that way. > RWF_* ? Isn't that kernel space flags? Or did you intend to say IOCB_FLAG_*? If yes, we maintain two flag fields? aio_reserved1 (perhaps renamed to aio_flags2) and aio_flags? aio_reserved1 is also used to return key for the purpose of io_cancel, but we should be able to fetch the flags before putting the key value there. Still I am not comfortable using the same field for it because it will be overwritten when io_submit returns. Which brings me to the next question: What is the purpose of aio_key? Why is aio_key set to KIOCB_KEY (which is zero) every time? You are not differentiating the request by setting all the iocb's key to zero.
On Wed, Mar 01, 2017 at 10:57:17AM -0600, Goldwyn Rodrigues wrote: > RWF_* ? Isn't that kernel space flags? Or did you intend to say > IOCB_FLAG_*? No, they are the flags for preadv2/pwritev2. > If yes, we maintain two flag fields? aio_reserved1 (perhaps > renamed to aio_flags2) and aio_flags? Yes - I'd call it aio_rw_flags or similar. > aio_reserved1 is also used to return key for the purpose of io_cancel, > but we should be able to fetch the flags before putting the key value > there. Still I am not comfortable using the same field for it because it > will be overwritten when io_submit returns. It's not - the key is a separate field. It's just that the two are defined using a very strange macro switching around their positions based on the endiannes. > Which brings me to the next question: What is the purpose of aio_key? > Why is aio_key set to KIOCB_KEY (which is zero) every time? You are not > differentiating the request by setting all the iocb's key to zero. I don't know the history of this rather odd field.
diff --git a/fs/aio.c b/fs/aio.c index 873b4ca..5ae19ba 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -1586,6 +1586,9 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb, req->common.ki_flags |= IOCB_EVENTFD; } + if (iocb->aio_flags & IOCB_FLAG_NOWAIT) + req->common.ki_flags |= IOCB_NOWAIT; + ret = put_user(KIOCB_KEY, &user_iocb->aio_key); if (unlikely(ret)) { pr_debug("EFAULT: aio_key\n"); diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba0743..ab2f556 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -270,6 +270,7 @@ struct writeback_control; #define IOCB_DSYNC (1 << 4) #define IOCB_SYNC (1 << 5) #define IOCB_WRITE (1 << 6) +#define IOCB_NOWAIT (1 << 7) struct kiocb { struct file *ki_filp; diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h index bb2554f..82d1d94 100644 --- a/include/uapi/linux/aio_abi.h +++ b/include/uapi/linux/aio_abi.h @@ -51,8 +51,11 @@ enum { * * IOCB_FLAG_RESFD - Set if the "aio_resfd" member of the "struct iocb" * is valid. + * IOCB_FLAG_NOWAIT - Set if the user wants the iocb to fail if it would block + * for operations such as disk allocation. */ #define IOCB_FLAG_RESFD (1 << 0) +#define IOCB_FLAG_NOWAIT (1 << 1) /* read() from /dev/aio returns these structures. */ struct io_event {