| Message ID | 20220519144841.784780-8-afaria@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement bdrv_{pread, pwrite, pwrite_sync, pwrite_zeroes}() using generated_co_wrapper | expand |
On Thu, May 19, 2022 at 03:48:37PM +0100, Alberto Faria wrote: > bdrv_pwrite_zeroes() now also calls trace_bdrv_co_pwrite_zeroes() and > clears the BDRV_REQ_MAY_UNMAP flag when appropriate, which it didn't > previously. The bdrv_pread()/bdrv_pwrite() errno for negative bytes changes from EINVAL to EIO. Did you audit the code to see if it matters?
On Thu, May 26, 2022 at 9:55 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > The bdrv_pread()/bdrv_pwrite() errno for negative bytes changes from > EINVAL to EIO. Did you audit the code to see if it matters? I don't believe I had, but I checked all calls now. There's ~140 of them, so the probability of me having overlooked something isn't exactly low, but it seems callers either cannot pass in negative values or don't care about the particular error code returned. Another option is to make bdrv_co_pread() and bdrv_co_pwrite() (which have much fewer callers) fail with -EINVAL when bytes is negative, but perhaps just getting rid of this final inconsistency between bdrv_[co_]{pread,pwrite}[v]() now will be worth it in the long run.
On Thu, May 26, 2022 at 08:23:02PM +0100, Alberto Faria wrote: > On Thu, May 26, 2022 at 9:55 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > The bdrv_pread()/bdrv_pwrite() errno for negative bytes changes from > > EINVAL to EIO. Did you audit the code to see if it matters? > > I don't believe I had, but I checked all calls now. There's ~140 of > them, so the probability of me having overlooked something isn't > exactly low, but it seems callers either cannot pass in negative > values or don't care about the particular error code returned. > > Another option is to make bdrv_co_pread() and bdrv_co_pwrite() (which > have much fewer callers) fail with -EINVAL when bytes is negative, but > perhaps just getting rid of this final inconsistency between > bdrv_[co_]{pread,pwrite}[v]() now will be worth it in the long run. Failing with -EINVAL for negative bytes makes more sense at identifying a programming error (whereas EIO tends to mean hardware failure), so making that sort of cleanup seems reasonable.
On Fri, May 27, 2022 at 09:25:06AM -0500, Eric Blake wrote: > On Thu, May 26, 2022 at 08:23:02PM +0100, Alberto Faria wrote: > > On Thu, May 26, 2022 at 9:55 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > The bdrv_pread()/bdrv_pwrite() errno for negative bytes changes from > > > EINVAL to EIO. Did you audit the code to see if it matters? > > > > I don't believe I had, but I checked all calls now. There's ~140 of > > them, so the probability of me having overlooked something isn't > > exactly low, but it seems callers either cannot pass in negative > > values or don't care about the particular error code returned. > > > > Another option is to make bdrv_co_pread() and bdrv_co_pwrite() (which > > have much fewer callers) fail with -EINVAL when bytes is negative, but > > perhaps just getting rid of this final inconsistency between > > bdrv_[co_]{pread,pwrite}[v]() now will be worth it in the long run. > > Failing with -EINVAL for negative bytes makes more sense at > identifying a programming error (whereas EIO tends to mean hardware > failure), so making that sort of cleanup seems reasonable. I'm surprised -EIO is being returned but all the more reason to check what effect changing to -EINVAL has. If you find it's safe to change to -EINVAL then that's consistent with how file I/O syscalls work and I think it would be nice. Stefan
On Fri, May 27, 2022 at 09:25:06AM -0500, Eric Blake wrote: > On Thu, May 26, 2022 at 08:23:02PM +0100, Alberto Faria wrote: > > On Thu, May 26, 2022 at 9:55 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > The bdrv_pread()/bdrv_pwrite() errno for negative bytes changes from > > > EINVAL to EIO. Did you audit the code to see if it matters? > > > > I don't believe I had, but I checked all calls now. There's ~140 of > > them, so the probability of me having overlooked something isn't > > exactly low, but it seems callers either cannot pass in negative > > values or don't care about the particular error code returned. > > > > Another option is to make bdrv_co_pread() and bdrv_co_pwrite() (which > > have much fewer callers) fail with -EINVAL when bytes is negative, but > > perhaps just getting rid of this final inconsistency between > > bdrv_[co_]{pread,pwrite}[v]() now will be worth it in the long run. > > Failing with -EINVAL for negative bytes makes more sense at > identifying a programming error (whereas EIO tends to mean hardware > failure), so making that sort of cleanup seems reasonable. From IRC: 13:50 < stefanha> kwolf hreitz: Is there a reason why bdrv_check_qiov_request() fails with -EIO instead of -EINVAL when parameters are invalid? 13:51 < hreitz> I think the reason is “EIO is kind of the default error value in the block layer” 13:53 < stefanha> bdrv_pwrite() has its own if (bytes < 0) return -EINVAL check, duplicating the input validation (but returning a different errno). 13:54 < hreitz> I think I’m only responsible for blk_check_byte_request(), but AFAIR that was my reasoning there 13:54 < stefanha> That makes me wonder if something depends on the exact errno. 13:54 < hreitz> I hope not 13:54 < stefanha> ...and what would break if it was changed to be EINVAL (consistent with file I/O syscalls). 13:55 < hreitz> Speaking for myself, I don’t think I’ve ever spent much consideration on what error codes to use in the block layer… 13:55 < kwolf> My guess is that it has always been EIO and nobody was bothered enough to check whether returning EINVAL instead would break anything 13:55 < stefanha> Thanks! 13:55 < hreitz> E2BIG might be special, and EAGAIN might do funny things sometimes, but other than that I’d’ve hoped everything’s treated equally 13:55 < stefanha> I'll add this information to the afaria's patch series discussion.
For completeness, a few more lines from IRC: 13:56 < hreitz> Errr s/E2BIG/ENOSPC/ 13:57 < kwolf> Anthony added it like this in 71d0770c4ce, and then we only had extensions and refactorings 13:58 < kwolf> Yes, ENOSPC actually has a different meaning because of the werror/rerror stuff 13:58 < kwolf> Well, only werror for ENOSPC
Thanks for the feedback, and apologies for the delayed response. On Mon, May 30, 2022 at 1:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > If you find it's safe to change to -EINVAL then that's consistent with > how file I/O syscalls work and I think it would be nice. Switching to -EINVAL on negative bytes sounds good to me, but perhaps it should be done as a separate series. For now, switching just bdrv_{pread,pwrite}() to -EIO will make them consistent with all of bdrv_{preadv,pwritev}() and bdrv_co_{pread,pwrite,preadv,pwritev}(), accomplishing the purpose of this series with less changes and auditing. I can work on a subsequent series that changes -EIO to -EINVAL on negative bytes for all the bdrv_...() and blk_...() functions. Would this make sense? Alberto
On Mon, Jun 06, 2022 at 05:10:38PM +0100, Alberto Faria wrote: > Thanks for the feedback, and apologies for the delayed response. > > On Mon, May 30, 2022 at 1:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > If you find it's safe to change to -EINVAL then that's consistent with > > how file I/O syscalls work and I think it would be nice. > > Switching to -EINVAL on negative bytes sounds good to me, but perhaps > it should be done as a separate series. For now, switching just > bdrv_{pread,pwrite}() to -EIO will make them consistent with all of > bdrv_{preadv,pwritev}() and bdrv_co_{pread,pwrite,preadv,pwritev}(), > accomplishing the purpose of this series with less changes and > auditing. > > I can work on a subsequent series that changes -EIO to -EINVAL on > negative bytes for all the bdrv_...() and blk_...() functions. > > Would this make sense? Yes, that's fine. My main concern is that callers have been audited when errnos are changed. If you switch bdrv_{pread,pwrite}() to -EIO and have audited callers, then I'm happy. Consistent -EINVAL would be nice in the future, but I think it's lower priority and it doesn't have to be done any time soon. Stefan
On Wed, Jun 8, 2022 at 1:50 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > Yes, that's fine. My main concern is that callers have been audited when > errnos are changed. If you switch bdrv_{pread,pwrite}() to -EIO and have > audited callers, then I'm happy. > > Consistent -EINVAL would be nice in the future, but I think it's lower > priority and it doesn't have to be done any time soon. Great. I'll send a v4 with the small change to patch 06/10 that remains, and note in the email for this patch (07/10) that it required quite a bit of auditing. As mentioned, there were ~140 call sites, so I'm not positive I didn't make a mistake. Hopefully someone more accustomed to the code base will have enough time to double-check this.
diff --git a/block/io.c b/block/io.c index 78a289192e..ecd1c2a53c 100644 --- a/block/io.c +++ b/block/io.c @@ -1061,14 +1061,6 @@ static int bdrv_check_request32(int64_t offset, int64_t bytes, return 0; } -int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset, - int64_t bytes, BdrvRequestFlags flags) -{ - IO_CODE(); - return bdrv_pwritev(child, offset, bytes, NULL, - BDRV_REQ_ZERO_WRITE | flags); -} - /* * Completely zero out a block device with the help of bdrv_pwrite_zeroes. * The operation is sped up by checking the block status and only writing @@ -1111,39 +1103,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) } } -/* See bdrv_pwrite() for the return codes */ -int bdrv_pread(BdrvChild *child, int64_t offset, int64_t bytes, void *buf, - BdrvRequestFlags flags) -{ - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes); - IO_CODE(); - - if (bytes < 0) { - return -EINVAL; - } - - return bdrv_preadv(child, offset, bytes, &qiov, flags); -} - -/* Return no. of bytes on success or < 0 on error. Important errors are: - -EIO generic I/O error (may happen for all errors) - -ENOMEDIUM No media inserted. - -EINVAL Invalid offset or number of bytes - -EACCES Trying to write a read-only device -*/ -int bdrv_pwrite(BdrvChild *child, int64_t offset, int64_t bytes, - const void *buf, BdrvRequestFlags flags) -{ - QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes); - IO_CODE(); - - if (bytes < 0) { - return -EINVAL; - } - - return bdrv_pwritev(child, offset, bytes, &qiov, flags); -} - /* * Writes to the file and ensures that no writes are reordered across this * request (acts as a barrier) diff --git a/include/block/block-io.h b/include/block/block-io.h index 879221cebe..c81739ad16 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -39,13 +39,16 @@ * to catch when they are accidentally called by the wrong API. */ -int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset, - int64_t bytes, BdrvRequestFlags flags); +int generated_co_wrapper bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset, + int64_t bytes, + BdrvRequestFlags flags); int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags); -int bdrv_pread(BdrvChild *child, int64_t offset, int64_t bytes, void *buf, - BdrvRequestFlags flags); -int bdrv_pwrite(BdrvChild *child, int64_t offset, int64_t bytes, - const void *buf, BdrvRequestFlags flags); +int generated_co_wrapper bdrv_pread(BdrvChild *child, int64_t offset, + int64_t bytes, void *buf, + BdrvRequestFlags flags); +int generated_co_wrapper bdrv_pwrite(BdrvChild *child, int64_t offset, + int64_t bytes, const void *buf, + BdrvRequestFlags flags); int bdrv_pwrite_sync(BdrvChild *child, int64_t offset, int64_t bytes, const void *buf, BdrvRequestFlags flags); /*