Re: [PATCH v5 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
Hi Hannes,
Thanks for the feedback...
On Sat, 1 Dec 2018 15:59:25 +0100, Hannes Reinecke wrote:
> On 12/1/18 12:34 AM, David Disseldorp wrote:
...
> > @@ -810,6 +810,23 @@ struct se_device *target_alloc_device(struct se_hba
> > *hba, const char *name)
> > mutex_init(_lun->lun_tg_pt_md_mutex);
> > xcopy_lun->lun_tpg = _pt_tpg;
> >
> > + /*
> > +* Preload the initial INQUIRY const values if we are doing
> > +* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
> > +* passthrough because this is being provided by the backend LLD.
> > +*/
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> > + BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
> > + if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> > + strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > + sizeof(dev->t10_wwn.vendor));
> > + strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > + sizeof(dev->t10_wwn.model));
> > + strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > + sizeof(dev->t10_wwn.revision));
> > + }
> > +
> > return dev;
> > }
> >
> This is odd. I'd rather have it consistent across backends, ie either
> move the initialisation into the backends, or provide a means to check
> if the inquiry data has already been pre-filled.
> But this check really is awkward.
Not quite sure I follow here. I could the default setting to the
target_backend_ops.alloc_device() code paths, but I don't think the
duplication would make this much cleaner, if at all.
I can look into this further if you like (target_backend_ops.inquiry_rev
could be dropped that way), but my preference would be to do so as a
follow-up patch-set.
Cheers, David
Re: DISABLE_CLUSTERING in scsi drivers
On Sun, 2 Dec 2018, Hannes Reinecke wrote: > On 12/2/18 10:21 PM, Finn Thain wrote: > > On Sun, 2 Dec 2018, Hannes Reinecke wrote: > > > > > Well, that lone 'kmap' is due to a quirk/errata in the datasheet; > > > essentially > > > we have to PIO a lone byte out of the FIFO to clear it up. > > > And this byte is technically still part of the SCSI data, so we need to > > > stuff it onto the end of the actual data sg list. Which is what the kmap() > > > thingie does. > > > So it really shouldn't be affected by the clustering algorithm. > > > > > > > Sorry, I don't follow. > > > > If it's dead code, can it be removed > > > Oh, it's not dead code. It's required as per datasheet. > > > If it's not, does it require DISABLE_CLUSTERING? > > > No, not really. It just affects the very last byte of the sglist, > so I can't really see how it should be affected by clustering. > AIUI, the scsi_kmap_atomic_sg() call which you added to esp_scsi.c assumes that the sg list elements are page sized and page aligned. DISABLE_CLUSTERING provides that guarantee, but am53c974.c doesn't use it. Is this not a bug? What am I missing? BTW, Documentation/block/biodoc.txt suggests a bounce buffer as an alternative. There are some situations when pages from high memory may need to be kmapped, even if bounce buffers are not necessary. For example a device may need to abort DMA operations and revert to PIO for the transfer, in which case a virtual mapping of the page is required. For SCSI it is also done in some scenarios where the low level driver cannot be trusted to handle a single sg entry correctly. The driver is expected to perform the kmaps as needed on such occasions as appropriate. A driver could also use the blk_queue_bounce() routine on its own to bounce highmem i/o to low memory for specific requests if so desired. -- > Cheers, > > Hannes > >
Re: DISABLE_CLUSTERING in scsi drivers
On 12/2/18 10:21 PM, Finn Thain wrote: On Sun, 2 Dec 2018, Hannes Reinecke wrote: Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Sorry, I don't follow. If it's dead code, can it be removed Oh, it's not dead code. It's required as per datasheet. If it's not, does it require DISABLE_CLUSTERING? No, not really. It just affects the very last byte of the sglist, so I can't really see how it should be affected by clustering. Cheers, Hannes
Re: DISABLE_CLUSTERING in scsi drivers
On Sun, 2 Dec 2018, Hannes Reinecke wrote: > Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially > we have to PIO a lone byte out of the FIFO to clear it up. > And this byte is technically still part of the SCSI data, so we need to > stuff it onto the end of the actual data sg list. Which is what the kmap() > thingie does. > So it really shouldn't be affected by the clustering algorithm. > Sorry, I don't follow. If it's dead code, can it be removed? If it's not, does it require DISABLE_CLUSTERING? -- > Cheers, > > Hannes >
Re: DISABLE_CLUSTERING in scsi drivers
On 11/26/18 10:46 AM, Finn Thain wrote: On Mon, 26 Nov 2018, Christoph Hellwig wrote: On Thu, Nov 22, 2018 at 09:02:13AM +1100, Finn Thain wrote: you in the To list maintain or wrote SCSI drivers that set the DISABLE_CLUSTERING flag, which basically disable merges of any bio segments. We already have the actual max_segment size limit to say which length a segment should have, independent of merged or originally created, so this limit generally should rarely if ever be required, and mostly is an old cut an paste error. Are you referring to blk_queue_max_segment_size(q, dma_get_max_seg_size(dev)); in drivers/scsi/scsi_lib.c? Is the segment size limitation of the DMA controller the only reason to want DISABLE_CLUSTERING? DISABLE_CLUSTERING mixes up two not really related things: 1) limit the size of each segment to a single page size 2) limit each segment to not actually span a page boundary. Both could be valid limit for DMA engines, but also might be particularly relevant for pio, if you e.g. kmap each page of a scatterlist do do pio you'd want to see both limits. I looked through all the drivers based on esp_scsi.c and NCR5380.c which use DISABLE_CLUSTERING. There is one driver that uses DMA and sometimes kmap too, which is am53c974.c. It defaults to ENABLE_CLUSTERING, which would seem to be a bug if kmap isn't compatible with that (Hannes?). Well, that lone 'kmap' is due to a quirk/errata in the datasheet; essentially we have to PIO a lone byte out of the FIFO to clear it up. And this byte is technically still part of the SCSI data, so we need to stuff it onto the end of the actual data sg list. Which is what the kmap() thingie does. So it really shouldn't be affected by the clustering algorithm. Cheers, Hannes
