Re: [PATCH v7 10/38] sg: improve naming

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-02-28 3:27 a.m., Hannes Reinecke wrote:
> On 2/27/20 5:58 PM, Douglas Gilbert wrote:
>> Remove use of typedef sg_io_hdr_t and replace with struct
>> sg_io_hdr. Change some names on driver wide structure fields
>> and comment them. Rename sg_alloc() to sg_add_device_helper()
>> to reflect its current role.
>>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>> ---
>>   drivers/scsi/sg.c | 240 ++++++++++++++++++++++++----------------------
>>   1 file changed, 127 insertions(+), 113 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 246c630d3523..eb3b333ea30b 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -92,7 +92,7 @@ static int sg_allow_dio = SG_ALLOW_DIO_DEF;
>>   static int scatter_elem_sz = SG_SCATTER_SZ;
>>   static int scatter_elem_sz_prev = SG_SCATTER_SZ;
>> -#define SG_SECTOR_SZ 512
>> +#define SG_DEF_SECTOR_SZ 512
>>   static int sg_add_device(struct device *, struct class_interface *);
>>   static void sg_remove_device(struct device *, struct class_interface *);
>> @@ -105,12 +105,13 @@ static struct class_interface sg_interface = {
>>       .remove_dev     = sg_remove_device,
>>   };
>> -struct sg_scatter_hold { /* holding area for scsi scatter gather info */
>> -    u16 k_use_sg; /* Count of kernel scatter-gather pieces */
>> +struct sg_scatter_hold {     /* holding area for scsi scatter gather info */
>> +    struct page **pages;    /* num_sgat element array of struct page* */
>> +    int buflen;        /* capacity in bytes (dlen<=buflen) */
>> +    int dlen;        /* current valid data length of this req */
>> +    u16 page_order;        /* byte_len = (page_size*(2**page_order)) */
>> +    u16 num_sgat;        /* actual number of scatter-gather segments */
>>       unsigned int sglist_len; /* size of malloc'd scatter-gather list ++ */
>> -    unsigned int bufflen;    /* Size of (aggregate) data buffer */
>> -    struct page **pages;
>> -    int page_order;
>>       char dio_in_use;    /* 0->indirect IO (or mmap), 1->dio */
>>       u8 cmd_opcode;        /* first byte of command */
>>   };
> While you're at it, it might be worthwhile to exchange the order of the last two 
> fields.
> Having a 'char' before the 'u8' guarantees either a misaligned structure or 
> implicit padding by the compiler, neither of which is a good thing.

dio_in_use became part of a bitfield and is now gone. I find keeping the
opcode (first byte of the opcode) around very useful for debugging. As for
alignment, we could keep the first 4 bytes of the cdb instead :-)
struct sg_scatter_hold is embedded in struct sg_request so unless there
is another 3 bytes of u8s to put beside cmd_opcode, then there is going
to be padding somewhere.

>> @@ -122,20 +123,20 @@ struct sg_request {    /* SG_MAX_QUEUE requests 
>> outstanding per file */
>>       struct list_head entry;    /* list entry */
>>       struct sg_fd *parentfp;    /* NULL -> not in use */
>>       struct sg_scatter_hold data;    /* hold buffer, perhaps scatter list */
>> -    sg_io_hdr_t header;    /* scsi command+info, see <scsi/sg.h> */
>> +    struct sg_io_hdr header;  /* scsi command+info, see <scsi/sg.h> */
>>       u8 sense_b[SCSI_SENSE_BUFFERSIZE];
>>       char res_used;        /* 1 -> using reserve buffer, 0 -> not ... */
>>       char orphan;        /* 1 -> drop on sight, 0 -> normal */
>>       char sg_io_owned;    /* 1 -> packet belongs to SG_IO */
>>       /* done protected by rq_list_lock */
>>       char done;        /* 0->before bh, 1->before read, 2->read */
> 
> And here a bitfield might be a good idea, as otherwise you might run into 
> alignment / padding issues again ...

They are indeed put in a bitfield in a later patch. Another reviewer has
complained that I tend to "sneak" small changes in that have little to
do with the title of the patch (guilty). Hard to keep everyone happy.

Doug Gilbert