Re: Linux 5.3-rc1

[Steffen Maier]

On 7/23/19 7:28 AM, James Bottomley wrote:
> On Mon, 2019-07-22 at 19:42 -0700, Guenter Roeck wrote:
> > On 7/22/19 4:45 PM, James Bottomley wrote:
> > > [linux-scsi added to cc]
> > > On Mon, 2019-07-22 at 15:21 -0700, Guenter Roeck wrote:
> > > > On Sun, Jul 21, 2019 at 02:33:38PM -0700, Linus Torvalds wrote:
> > > >
> > > > [ ... ]
> > > > > Go test,
> > > >
> > > > Things looked pretty good until a few days ago. Unfortunately,
> > > > the last few days brought in a couple of issues.
> > > >
> > > > riscv:virt:defconfig:scsi[virtio]
> > > > riscv:virt:defconfig:scsi[virtio-pci]
> > > >
> > > > Boot tests crash with no useful backtrace. Bisect points to
> > > > merge ac60602a6d8f ("Merge tag 'dma-mapping-5.3-1'"). Log is at
> > > > https://kerneltests.org/builders/qemu-riscv64-master/builds/238/s
> > > > teps
> > > > /qemubuildcommand_1/logs/stdio
> > > >
> > > > ppc:mpc8544ds:mpc85xx_defconfig:sata-sii3112
> > > > ppc64:pseries:pseries_defconfig:sata-sii3112
> > > > ppc64:pseries:pseries_defconfig:little:sata-sii3112
> > > > ppc64:ppce500:corenet64_smp_defconfig:e5500:sata-sii3112
> > > >
> > > > ata1: lost interrupt (Status 0x50)
> > > > ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
> > > > ata1.00: failed command: READ DMA
> > > >
> > > > and many similar errors. Boot ultimately times out. Bisect points
> > > > to
> > > > merge
> > > > f65420df914a ("Merge tag 'scsi-fixes'").
> > > >
> > > > Logs:
> > > > https://kerneltests.org/builders/qemu-ppc64-master/builds/1212/st
> > > > eps/
> > > > qemubuildcommand/logs/stdio
> > > > https://kerneltests.org/builders/qemu-ppc-master/builds/1255/step
> > > > s/qe
> > > > mubuildcommand/logs/stdio
> > > >
> > > > Guenter
> > > >
> > > > ---
> > > > riscv bisect log
> > > >
> > > > # bad: [5f9e832c137075045d15cd6899ab0505cfb2ca4b] Linus 5.3-rc1
> > > > # good: [bdd17bdef7d8da4d8eee254abb4c92d8a566bdc1] scsi: core:
> > > > take
> > > > the DMA max mapping size into account

> > > > # first bad commit: [ac60602a6d8f6830dee89f4b87ee005f62eb7171]
> > > > Merge
> > > > tag 'dma-mapping-5.3-1' of git://git.infradead.org/users/hch/dma-
> > > > mapping

> > > When a bisect lands on a merge commit it usually indicates bad
> > > interaction between two trees.  The way to find it is to do a
> > > bisect,
> > > but merge up to the other side of the scsi-fixes pull before
> > > running
> > > tests so the interaction is exposed in the bisect.
> >
> > Can you provide instructions for dummies ?
>
> do a man git-bisect and then follow the 'Automatically bisect with
> temporary modifications' example.  You substitute
> 168c79971b4a7be7011e73bf488b740a8e1135c8 for hot-fix
>
> > > However my money is on:
> > >
> > >
> > > commit bdd17bdef7d8da4d8eee254abb4c92d8a566bdc1
> > > Author: Christoph Hellwig <hch@xxxxxx>
> > > Date:   Mon Jun 17 14:19:54 2019 +0200
> > >
> > >       scsi: core: take the DMA max mapping size into account
> > >    
> > > Now that I look at the code again:
> > >
> > >
> > > +       shost->max_sectors = min_t(unsigned int, shost-
> > > > max_sectors,
> > > +                       dma_max_mapping_size(dev) << SECTOR_SHIFT);
> > >
> > > That shift looks to be the wrong way around (should be >>).  I bet
> > > something is giving a very large number which becomes zero on left
> > > shift, meaning max_sectors gets set to zero.
> >
> > That does indeed look bad, but changing it doesn't make a difference.
>
> Odd, all the other changes are driver specific (and not in ATA) apart
> from this one:
>
> commit 7ad388d8e4c703980b7018b938cdeec58832d78d
> Author: Christoph Hellwig <hch@xxxxxx>
> Date:   Mon Jun 17 14:19:53 2019 +0200
>
>      scsi: core: add a host / host template field for the virt boundary
>
>
> I suppose it could be because the virt_boundary_mask isn't set, but
> that should just set zero, which is what block usually does.

I found max_segment_size unexpectedly to be UINT_MAX with zfcp today in our CI. 
My investigations are still very early, but I thought, I share a few thoughts 
as I'm way too unfamiliar with the DMA business and thus hope for help.

Above commit introduced an unconditional call to blk_queue_virt_boundary(q, 
shost->virt_boundary_mask), _after_ blk_queue_max_segment_size(q, 
shost->max_segment_size).

Looking at the source, dma_set_max_seg_size() seems to unconditionally 
overwrite max_segment_size:

> /**
>  * blk_queue_virt_boundary - set boundary rules for bio merging
>  * @q:  the request queue for the device
>  * @mask:  the memory boundary mask
>  **/
> void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
> {
> 	q->limits.virt_boundary_mask = mask;
>
> 	/*
> 	 * Devices that require a virtual boundary do not support scatter/gather
> 	 * I/O natively, but instead require a descriptor list entry for each
> 	 * page (which might not be idential to the Linux PAGE_SIZE).  Because
> 	 * of that they are not limited by our notion of "segment size".
> 	 */
> 	q->limits.max_segment_size = UINT_MAX;
> }
> EXPORT_SYMBOL(blk_queue_virt_boundary);

Wild guess: Do we need to make the call to blk_queue_virt_boundary() conditional?

Cf. https://www.spinics.net/lists/linux-scsi/msg131077.html ("[PATCH v2] iser: 
explicitly set shost max_segment_size if non virtual boundary devices")

--
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294