Re: [PATCH v2 2/2] block: cope with WRITE ZEROES failing in blkdev_issue_zeroout()

On Fri, Oct 6, 2017 at 2:05 PM, Christoph Hellwig [off-list ref] wrote:

On Thu, Oct 05, 2017 at 09:32:33PM +0200, Ilya Dryomov wrote:

quoted

This is to avoid returning -EREMOTEIO in the following case: device
doesn't support WRITE SAME but scsi_disk::max_ws_blocks != 0, zeroout
is called with BLKDEV_ZERO_NOFALLBACK.  Enter blkdev_issue_zeroout(),
bdev_write_zeroes_sectors() != 0, so we issue WRITE ZEROES.  The
request fails with ILLEGAL REQUEST, sd_done() sets ->no_write_same and
updates queue_limits, ILLEGAL REQUEST is translated into -EREMOTEIO,
which is returned from submit_bio_wait().  Manual zeroing is not
allowed, so we must return an error, but it shouldn't be -EREMOTEIO if
queue_limits just got updated because of ILLEGAL REQUEST.  Without this
conditional, we'd get

Hmm.  I think we'd better off to just do the before the retry loop:

        if (ret && try_write_zeroes) {
                if (!(flags & BLKDEV_ZERO_NOFALLBACK))
                        try_write_zeroes = false;
                        goto retry;
                }
                ret = -EOPNOTSUPP;
        }

This would unconditionally overwrite any WRITE ZEROS error.  If we get
e.g. -EIO, and manual zeroing is not allowed, I don't think we want to
return -EOPNOTSUPP?

Returning -EOPNOTSUPP to mean "can't zero using either method" doesn't
make sense to me, because manual zeroing is always supported, just not
always allowed.

Thanks,

                Ilya