Re: [PATCH] bsg : Add support for io vectors in bsg

[Pete Wyckoff <pw@osc.edu>]

tomof@acm.org wrote on Sat, 05 Jan 2008 14:01 +0900:
> From: Deepak Colluru <deepakrc@gmail.com>
> Subject: [PATCH] bsg : Add support for io vectors in bsg
> Date: Fri, 4 Jan 2008 21:47:34 +0530 (IST)
> 
> > From: Deepak Colluru <deepakrc@gmail.com>
> > 
> > Add support for io vectors in bsg.
> > 
> > Signed-off-by: Deepak Colluru <deepakrc@gmail.com>
> > ---
> >   bsg.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 49 insertions(+), 3 deletions(-)
> 
> Thanks, but I have to NACK this.
> 
> You can find the discussion about bsg io vector support and a similar
> patch in linux-scsi archive. I have no plan to support it since it
> needs the compat hack.

You may recall this is one of the patches I need to use bsg with OSD
devices.  OSDs overload the SCSI buffer model to put mulitple fields
in dataout and datain.  Some is user data, but some is more
logically created by a library.  Memcpying in userspace to wedge all
the segments into a single buffer is painful, and is required both
on outgoing and incoming data buffers.

There are two approaches to add iovec to bsg.

1.  Define a new sg_iovec_v4 that uses constant width types.  Both
    32- and 64-bit userspace would hand arrays of this to the kernel.

    struct sg_v4_iovec {
	    __u64 iov_base;
	    __u32 iov_len;
	    __u32 __pad1;
    };

    Old patch here:  http://article.gmane.org/gmane.linux.scsi/30461/


2.  Do as Deepak has done, using the existing sg_iovec, but then
    also work around the compat issue.  Old v3 sg_iovec is:

    typedef struct sg_iovec /* same structure as used by readv() Linux system */
    {                       /* call. It defines one scatter-gather element. */
	void __user *iov_base;      /* Starting address  */
	size_t iov_len;             /* Length in bytes  */
    } sg_iovec_t;

    Old patch here:  http://article.gmane.org/gmane.linux.scsi/30460/

I took another look at the compat approach, to see if it is feasible
to keep the compat handling somewhere else, without the use of #ifdef
CONFIG_COMPAT and size-comparison code inside bsg.c.  I don't see how.
The use of iovec is within a write operation on a char device.  It's
not amenable to a compat_sys_ or a .compat_ioctl approach.

I'm partial to #1 because the use of architecture-independent fields
matches the rest of struct sg_io_v4.  But if you don't want to have
another iovec type in the kernel, could we do #2 but just return
-EINVAL if the need for compat is detected?  I.e. change
dout_iovec_count to dout_iovec_length and do the math?

		-- Pete