Re: [Drbd-dev] [dm-devel] [PATCH 19/32] block: add helper to get data dir from op

Re: [Drbd-dev] [dm-devel] [PATCH 19/32] block: add helper to get data dir from op

[Bart Van Assche]

On 11/04/2015 02:08 PM, mchristi@redhat.com wrote:
> From: Mike Christie <mchristi@redhat.com>
>
> In later patches the op will no longer be a bitmap, so we will
> not have REQ_WRITE set for all non reads like discard, flush,
> and write same. Drivers will still want to treat them as writes
> for accounting reasons, so this patch adds a helper to translate
> a op to a data direction.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> ---
>   include/linux/blkdev.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 19c2e94..cf5f518 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -586,6 +586,18 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
>
>   #define list_entry_rq(ptr)	list_entry((ptr), struct request, queuelist)
>
> +/*
> + * Non REQ_OP_WRITE requests like discard, write same, etc, are
> + * considered WRITEs.
> + */
> +static inline int op_to_data_dir(int op)
> +{
> +	if (op == REQ_OP_READ)
> +		return READ;
> +	else
> +		return WRITE;
> +}
> +
>   #define rq_data_dir(rq)		((int)((rq)->cmd_flags & 1))
>
>   /*
>

How about introducing two functions - op_is_write() and op_is_read() ? I 
think that approach will result in shorter and easier to read code in 
the contexts where these functions are used.

Bart.

Re: [Drbd-dev] [dm-devel] [PATCH 19/32] block: add helper to get data dir from op

[Christoph Hellwig]

On Thu, Nov 05, 2015 at 11:34:07AM -0600, Mike Christie wrote:
> I can do that. You are right in how they are used. I just did the above,
> to follow the other *_data_dir calls.

I think the *_data_dir calls are horrible interfaces.  But your series
already is huge, so if it makes your life easier I'd say keep it as-is
for now.  But in the long run we should have the interfaces that Bart
suggested, and one that gives you a dma_data_direction from a request,
as that's what a lot of the driver ultimatively want.

Re: [Drbd-dev] [RESEND RFC PATCH 00/32] separate operations from flags in the bio/request structs

[Christoph Hellwig]

On Wed, Nov 04, 2015 at 04:07:57PM -0600, mchristi@redhat.com wrote:
> Known issues:
> - REQ_FLUSH is still a flag, but should probably be a operation.
>  For lower level drivers like SCSI where we only get a flush, it makes
> more sense to be a operation. However, upper layers like filesystems
> can send down flushes with writes, so it is more of a flag for them.
> I am still working on this.

Actually it should be both.  REQ_OP_FLUSH for a real flush operation,
and a REQ_PREFLUSH bio flag that the request layer will sequence into
an actual write an a flush operation.

Re: [Drbd-dev] [PATCH 06/32] xen blkback: prepare for bi_rw split

[Christoph Hellwig]

A little offtopic for this patch, but can some explain this whole
mess about bios in Xen blkfront?  We can happily do partial completions
at the request later.

Also since the blk-mq conversion the call to blk_end_request_all is
completely broken, so it doesn't look like this code path is exactly
well tested.

Re: [Drbd-dev] [PATCH 32/32] block: remove __REQ op defs and reduce bi_op/bi_rw sizes

[Christoph Hellwig]

I love this series!

Also I think it's very good that we finally split the ioprio out to it's
own field.

Thanks a lot for doing this work.

> I was not sure if or how much or where people wanted to stick things.
> There also appears to be room in the bi_flags field. If bi_flags is
> only using 13 bits and there are only 16 REQ_XYZs bits related bios,
> I could put them all in one variable if we wanted to go wild with trying
> to shrink the bio while I am at it..

This sounds interesting, but I'd rather do it as a separate project
later.