"s390/dasd: Remove DMA alignment" for stable

"s390/dasd: Remove DMA alignment" for stable

[Jan Höppner]

Hi,

the stable tag was missing for the following commit:
commit 2a07bb64d801 ("s390/dasd: Remove DMA alignment")

The change needs to be applied for kernel 6.0+ essentially reverting
bc792884b76f ("s390/dasd: Establish DMA alignment").

The patch fixes filesystem errors especially for XFS when DASD devices are formatted
with a blocksize smaller than 4096 bytes.

The commit 2a07bb64d801 ("s390/dasd: Remove DMA alignment") should apply cleanly for
kernel 6.9+. There was a refactoring happening at the time with the following two commits
(just for context, not required as prereqs!):
commit 0127a47f58c6 ("dasd: move queue setup to common code")
commit fde07a4d74e3 ("dasd: use the atomic queue limits API")

For everything before 6.9 a simple git revert for commit bc792884b76f
("s390/dasd: Establish DMA alignment") should work just fine.

If you run into any conflicts, need separate patches, or have any questions,
please let me know.

Thanks a lot and apologies for the inconvenience!

regards,
Jan

Re: "s390/dasd: Remove DMA alignment" for stable

[Greg KH]

On Tue, Aug 20, 2024 at 11:41:18AM +0200, Jan Höppner wrote:
> Hi,
> 
> the stable tag was missing for the following commit:
> commit 2a07bb64d801 ("s390/dasd: Remove DMA alignment")
> 
> The change needs to be applied for kernel 6.0+ essentially reverting
> bc792884b76f ("s390/dasd: Establish DMA alignment").
> 
> The patch fixes filesystem errors especially for XFS when DASD devices are formatted
> with a blocksize smaller than 4096 bytes.
> 
> The commit 2a07bb64d801 ("s390/dasd: Remove DMA alignment") should apply cleanly for
> kernel 6.9+. There was a refactoring happening at the time with the following two commits
> (just for context, not required as prereqs!):
> commit 0127a47f58c6 ("dasd: move queue setup to common code")
> commit fde07a4d74e3 ("dasd: use the atomic queue limits API")
> 
> For everything before 6.9 a simple git revert for commit bc792884b76f
> ("s390/dasd: Establish DMA alignment") should work just fine.
> 
> If you run into any conflicts, need separate patches, or have any questions,
> please let me know.

Can you please send the revert commit?  That makes it much simpler for
us to apply instead of relying on us to take the time doing it (hint, I
don't have time this week as I'm at a conference...)

thanks,

greg k-h

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

[Jan Höppner]

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(-)

diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
index 2e4e555b37c3..12db1046aad0 100644
--- a/drivers/s390/block/dasd_diag.c
+++ b/drivers/s390/block/dasd_diag.c
@@ -639,7 +639,6 @@ static void dasd_diag_setup_blk_queue(struct dasd_block *block)
 	/* With page sized segments each segment can be translated into one idaw/tidaw */
 	blk_queue_max_segment_size(q, PAGE_SIZE);
 	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
-	blk_queue_dma_alignment(q, PAGE_SIZE - 1);
 }
 
 static int dasd_diag_pe_handler(struct dasd_device *device,
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index bd89b032968a..18b63210ac5d 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -6895,7 +6895,6 @@ static void dasd_eckd_setup_blk_queue(struct dasd_block *block)
 	/* With page sized segments each segment can be translated into one idaw/tidaw */
 	blk_queue_max_segment_size(q, PAGE_SIZE);
 	blk_queue_segment_boundary(q, PAGE_SIZE - 1);
-	blk_queue_dma_alignment(q, PAGE_SIZE - 1);
 }
 
 static struct ccw_driver dasd_eckd_driver = {
-- 
2.43.0

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

[Greg KH]

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