* [PATCH 1/2] s390/dasd: Remove DMA alignment
2024-08-12 12:57 [PATCH 0/2] s390/dasd: fixes for data corruption and DMA alignment Stefan Haberland
@ 2024-08-12 12:57 ` Stefan Haberland
2024-08-12 12:57 ` [PATCH 2/2] s390/dasd: fix error recovery leading to data corruption on ESE devices Stefan Haberland
2024-08-12 16:31 ` [PATCH 0/2] s390/dasd: fixes for data corruption and DMA alignment Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Stefan Haberland @ 2024-08-12 12:57 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Jan Hoeppner, linux-s390, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Eric Farman
From: Eric Farman <farman@linux.ibm.com>
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. There were two commits [1]
that moved this code in the interim, so a straight git-revert is not
possible, but the change is straightforward.
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.
[1] commit 0127a47f58c6 ("dasd: move queue setup to common code")
commit fde07a4d74e3 ("dasd: use the atomic queue limits API")
Fixes: bc792884b76f ("s390/dasd: Establish DMA alignment")
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd_genhd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 1aa426b1dedd..6da47a65af61 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -41,7 +41,6 @@ int dasd_gendisk_alloc(struct dasd_block *block)
*/
.max_segment_size = PAGE_SIZE,
.seg_boundary_mask = PAGE_SIZE - 1,
- .dma_alignment = PAGE_SIZE - 1,
.max_segments = USHRT_MAX,
};
struct gendisk *gdp;
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] s390/dasd: fix error recovery leading to data corruption on ESE devices
2024-08-12 12:57 [PATCH 0/2] s390/dasd: fixes for data corruption and DMA alignment Stefan Haberland
2024-08-12 12:57 ` [PATCH 1/2] s390/dasd: Remove " Stefan Haberland
@ 2024-08-12 12:57 ` Stefan Haberland
2024-08-12 16:31 ` [PATCH 0/2] s390/dasd: fixes for data corruption and DMA alignment Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Stefan Haberland @ 2024-08-12 12:57 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Jan Hoeppner, linux-s390, Heiko Carstens,
Vasily Gorbik, Christian Borntraeger, Eric Farman
Extent Space Efficient (ESE) or thin provisioned volumes need to be
formatted on demand during usual IO processing.
The dasd_ese_needs_format function checks for error codes that signal
the non existence of a proper track format.
The check for incorrect length is to imprecise since other error cases
leading to transport of insufficient data also have this flag set.
This might lead to data corruption in certain error cases for example
during a storage server warmstart.
Fix by removing the check for incorrect length and replacing by
explicitly checking for invalid track format in transport mode.
Also remove the check for file protected since this is not a valid
ESE handling case.
Cc: <stable@vger.kernel.org> # 5.3+
Fixes: 5e2b17e712cf ("s390/dasd: Add dynamic formatting support for ESE volumes")
Reviewed-by: Jan Hoeppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
drivers/s390/block/dasd.c | 36 ++++++++++++-------
drivers/s390/block/dasd_3990_erp.c | 10 ++----
drivers/s390/block/dasd_eckd.c | 55 +++++++++++++-----------------
drivers/s390/block/dasd_int.h | 2 +-
4 files changed, 50 insertions(+), 53 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 0a97cfedd706..42a4a996defb 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -1601,9 +1601,15 @@ static int dasd_ese_needs_format(struct dasd_block *block, struct irb *irb)
if (!sense)
return 0;
- return !!(sense[1] & SNS1_NO_REC_FOUND) ||
- !!(sense[1] & SNS1_FILE_PROTECTED) ||
- scsw_cstat(&irb->scsw) == SCHN_STAT_INCORR_LEN;
+ if (sense[1] & SNS1_NO_REC_FOUND)
+ return 1;
+
+ if ((sense[1] & SNS1_INV_TRACK_FORMAT) &&
+ scsw_is_tm(&irb->scsw) &&
+ !(sense[2] & SNS2_ENV_DATA_PRESENT))
+ return 1;
+
+ return 0;
}
static int dasd_ese_oos_cond(u8 *sense)
@@ -1624,7 +1630,7 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
struct dasd_device *device;
unsigned long now;
int nrf_suppressed = 0;
- int fp_suppressed = 0;
+ int it_suppressed = 0;
struct request *req;
u8 *sense = NULL;
int expires;
@@ -1679,8 +1685,9 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
*/
sense = dasd_get_sense(irb);
if (sense) {
- fp_suppressed = (sense[1] & SNS1_FILE_PROTECTED) &&
- test_bit(DASD_CQR_SUPPRESS_FP, &cqr->flags);
+ it_suppressed = (sense[1] & SNS1_INV_TRACK_FORMAT) &&
+ !(sense[2] & SNS2_ENV_DATA_PRESENT) &&
+ test_bit(DASD_CQR_SUPPRESS_IT, &cqr->flags);
nrf_suppressed = (sense[1] & SNS1_NO_REC_FOUND) &&
test_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
@@ -1695,7 +1702,7 @@ void dasd_int_handler(struct ccw_device *cdev, unsigned long intparm,
return;
}
}
- if (!(fp_suppressed || nrf_suppressed))
+ if (!(it_suppressed || nrf_suppressed))
device->discipline->dump_sense_dbf(device, irb, "int");
if (device->features & DASD_FEATURE_ERPLOG)
@@ -2459,14 +2466,17 @@ static int _dasd_sleep_on_queue(struct list_head *ccw_queue, int interruptible)
rc = 0;
list_for_each_entry_safe(cqr, n, ccw_queue, blocklist) {
/*
- * In some cases the 'File Protected' or 'Incorrect Length'
- * error might be expected and error recovery would be
- * unnecessary in these cases. Check if the according suppress
- * bit is set.
+ * In some cases certain errors might be expected and
+ * error recovery would be unnecessary in these cases.
+ * Check if the according suppress bit is set.
*/
sense = dasd_get_sense(&cqr->irb);
- if (sense && sense[1] & SNS1_FILE_PROTECTED &&
- test_bit(DASD_CQR_SUPPRESS_FP, &cqr->flags))
+ if (sense && (sense[1] & SNS1_INV_TRACK_FORMAT) &&
+ !(sense[2] & SNS2_ENV_DATA_PRESENT) &&
+ test_bit(DASD_CQR_SUPPRESS_IT, &cqr->flags))
+ continue;
+ if (sense && (sense[1] & SNS1_NO_REC_FOUND) &&
+ test_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags))
continue;
if (scsw_cstat(&cqr->irb.scsw) == 0x40 &&
test_bit(DASD_CQR_SUPPRESS_IL, &cqr->flags))
diff --git a/drivers/s390/block/dasd_3990_erp.c b/drivers/s390/block/dasd_3990_erp.c
index bbbacfc386f2..d0aa267462c5 100644
--- a/drivers/s390/block/dasd_3990_erp.c
+++ b/drivers/s390/block/dasd_3990_erp.c
@@ -1386,14 +1386,8 @@ dasd_3990_erp_file_prot(struct dasd_ccw_req * erp)
struct dasd_device *device = erp->startdev;
- /*
- * In some cases the 'File Protected' error might be expected and
- * log messages shouldn't be written then.
- * Check if the according suppress bit is set.
- */
- if (!test_bit(DASD_CQR_SUPPRESS_FP, &erp->flags))
- dev_err(&device->cdev->dev,
- "Accessing the DASD failed because of a hardware error\n");
+ dev_err(&device->cdev->dev,
+ "Accessing the DASD failed because of a hardware error\n");
return dasd_3990_erp_cleanup(erp, DASD_CQR_FAILED);
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 9388b5c383ca..90b106408992 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2275,6 +2275,7 @@ dasd_eckd_analysis_ccw(struct dasd_device *device)
cqr->status = DASD_CQR_FILLED;
/* Set flags to suppress output for expected errors */
set_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
+ set_bit(DASD_CQR_SUPPRESS_IT, &cqr->flags);
return cqr;
}
@@ -2556,7 +2557,6 @@ dasd_eckd_build_check_tcw(struct dasd_device *base, struct format_data_t *fdata,
cqr->buildclk = get_tod_clock();
cqr->status = DASD_CQR_FILLED;
/* Set flags to suppress output for expected errors */
- set_bit(DASD_CQR_SUPPRESS_FP, &cqr->flags);
set_bit(DASD_CQR_SUPPRESS_IL, &cqr->flags);
return cqr;
@@ -4130,8 +4130,6 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_cmd_single(
/* Set flags to suppress output for expected errors */
if (dasd_eckd_is_ese(basedev)) {
- set_bit(DASD_CQR_SUPPRESS_FP, &cqr->flags);
- set_bit(DASD_CQR_SUPPRESS_IL, &cqr->flags);
set_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
}
@@ -4633,9 +4631,8 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
/* Set flags to suppress output for expected errors */
if (dasd_eckd_is_ese(basedev)) {
- set_bit(DASD_CQR_SUPPRESS_FP, &cqr->flags);
- set_bit(DASD_CQR_SUPPRESS_IL, &cqr->flags);
set_bit(DASD_CQR_SUPPRESS_NRF, &cqr->flags);
+ set_bit(DASD_CQR_SUPPRESS_IT, &cqr->flags);
}
return cqr;
@@ -5780,36 +5777,32 @@ static void dasd_eckd_dump_sense(struct dasd_device *device,
{
u8 *sense = dasd_get_sense(irb);
- if (scsw_is_tm(&irb->scsw)) {
- /*
- * In some cases the 'File Protected' or 'Incorrect Length'
- * error might be expected and log messages shouldn't be written
- * then. Check if the according suppress bit is set.
- */
- if (sense && (sense[1] & SNS1_FILE_PROTECTED) &&
- test_bit(DASD_CQR_SUPPRESS_FP, &req->flags))
- return;
- if (scsw_cstat(&irb->scsw) == 0x40 &&
- test_bit(DASD_CQR_SUPPRESS_IL, &req->flags))
- return;
+ /*
+ * In some cases certain errors might be expected and
+ * log messages shouldn't be written then.
+ * Check if the according suppress bit is set.
+ */
+ if (sense && (sense[1] & SNS1_INV_TRACK_FORMAT) &&
+ !(sense[2] & SNS2_ENV_DATA_PRESENT) &&
+ test_bit(DASD_CQR_SUPPRESS_IT, &req->flags))
+ return;
- dasd_eckd_dump_sense_tcw(device, req, irb);
- } else {
- /*
- * In some cases the 'Command Reject' or 'No Record Found'
- * error might be expected and log messages shouldn't be
- * written then. Check if the according suppress bit is set.
- */
- if (sense && sense[0] & SNS0_CMD_REJECT &&
- test_bit(DASD_CQR_SUPPRESS_CR, &req->flags))
- return;
+ if (sense && sense[0] & SNS0_CMD_REJECT &&
+ test_bit(DASD_CQR_SUPPRESS_CR, &req->flags))
+ return;
- if (sense && sense[1] & SNS1_NO_REC_FOUND &&
- test_bit(DASD_CQR_SUPPRESS_NRF, &req->flags))
- return;
+ if (sense && sense[1] & SNS1_NO_REC_FOUND &&
+ test_bit(DASD_CQR_SUPPRESS_NRF, &req->flags))
+ return;
+ if (scsw_cstat(&irb->scsw) == 0x40 &&
+ test_bit(DASD_CQR_SUPPRESS_IL, &req->flags))
+ return;
+
+ if (scsw_is_tm(&irb->scsw))
+ dasd_eckd_dump_sense_tcw(device, req, irb);
+ else
dasd_eckd_dump_sense_ccw(device, req, irb);
- }
}
static int dasd_eckd_reload_device(struct dasd_device *device)
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index e5f40536b425..81cfb5c89681 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -196,7 +196,7 @@ struct dasd_ccw_req {
* The following flags are used to suppress output of certain errors.
*/
#define DASD_CQR_SUPPRESS_NRF 4 /* Suppress 'No Record Found' error */
-#define DASD_CQR_SUPPRESS_FP 5 /* Suppress 'File Protected' error*/
+#define DASD_CQR_SUPPRESS_IT 5 /* Suppress 'Invalid Track' error*/
#define DASD_CQR_SUPPRESS_IL 6 /* Suppress 'Incorrect Length' error */
#define DASD_CQR_SUPPRESS_CR 7 /* Suppress 'Command Reject' error */
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread