Re: [PATCH] Revert "s390/dasd: Establish DMA alignment"

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Aug 20, 2024 at 04:13:07PM +0200, Jan Höppner wrote:
> This reverts commit bc792884b76f ("s390/dasd: Establish DMA alignment").
> 
> Quoting the original commit:
>     linux-next commit bf8d08532bc1 ("iomap: add support for dma aligned
>     direct-io") changes the alignment requirement to come from the block
>     device rather than the block size, and the default alignment
>     requirement is 512-byte boundaries. Since DASD I/O has page
>     alignments for IDAW/TIDAW requests, let's override this value to
>     restore the expected behavior.
> 
> I mentioned TIDAW, but that was wrong. TIDAWs have no distinct alignment
> requirement (per p. 15-70 of POPS SA22-7832-13):
> 
>    Unless otherwise specified, TIDAWs may designate
>    a block of main storage on any boundary and length
>    up to 4K bytes, provided the specified block does not
>    cross a 4 K-byte boundary.
> 
> IDAWs do, but the original commit neglected that while ECKD DASD are
> typically formatted in 4096-byte blocks, they don't HAVE to be. Formatting
> an ECKD volume with smaller blocks is permitted (dasdfmt -b xxx), and the
> problematic commit enforces alignment properties to such a device that
> will result in errors, such as:
> 
>    [test@host ~]# lsdasd -l a367 | grep blksz
>      blksz:				512
>    [test@host ~]# mkfs.xfs -f /dev/disk/by-path/ccw-0.0.a367-part1
>    meta-data=/dev/dasdc1            isize=512    agcount=4, agsize=230075 blks
>             =                       sectsz=512   attr=2, projid32bit=1
>             =                       crc=1        finobt=1, sparse=1, rmapbt=1
>             =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
>    data     =                       bsize=4096   blocks=920299, imaxpct=25
>             =                       sunit=0      swidth=0 blks
>    naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
>    log      =internal log           bsize=4096   blocks=16384, version=2
>             =                       sectsz=512   sunit=0 blks, lazy-count=1
>    realtime =none                   extsz=4096   blocks=0, rtextents=0
>    error reading existing superblock: Invalid argument
>    mkfs.xfs: pwrite failed: Invalid argument
>    libxfs_bwrite: write failed on (unknown) bno 0x70565c/0x100, err=22
>    mkfs.xfs: Releasing dirty buffer to free list!
>    found dirty buffer (bulk) on free list!
>    mkfs.xfs: pwrite failed: Invalid argument
>    ...snipped...
> 
> The original commit omitted the FBA discipline for just this reason,
> but the formatted block size of the other disciplines was overlooked.
> The solution to all of this is to revert to the original behavior,
> such that the block size can be respected.
> 
> But what of the original problem? That was manifested with a direct-io
> QEMU guest, where QEMU itself was changed a month or two later with
> commit 25474d90aa ("block: use the request length for iov alignment")
> such that the blamed kernel commit is unnecessary.
> 
> Note: This is an adapted version of the original upstream commit
> 2a07bb64d801 ("s390/dasd: Remove DMA alignment").
> 
> Cc: stable@vger.kernel.org # 6.0+
> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_diag.c | 1 -
>  drivers/s390/block/dasd_eckd.c | 1 -
>  2 files changed, 2 deletions(-)
> 

Now queued up, thanks.

greg k-h