| Message ID | 1372338695-411-11-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > if the target does not support UNMAP or the request > is too big silently ignore the discard request. > > Signed-off-by: Peter Lieven <pl@kamp.de> Why not loop for the "too big" case? You can probably use the same logic for unmapping the whole device in .bdrv_create and here. Kevin
Am 10.07.2013 13:33, schrieb Kevin Wolf: > Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: >> if the target does not support UNMAP or the request >> is too big silently ignore the discard request. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> > Why not loop for the "too big" case? You can probably use the same logic > for unmapping the whole device in .bdrv_create and here. right, but looping in an aio function seemed not so trivial to me. it seems more and more obvious to me that the best would be to change all the remaining aio routines to co routines. in this case i could add the too big logic in iscsi_co_discard and simply call it from iscsi_co_write_zeroes. Peter > > Kevin
Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben: > Am 10.07.2013 13:33, schrieb Kevin Wolf: > > Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > >> if the target does not support UNMAP or the request > >> is too big silently ignore the discard request. > >> > >> Signed-off-by: Peter Lieven <pl@kamp.de> > > Why not loop for the "too big" case? You can probably use the same logic > > for unmapping the whole device in .bdrv_create and here. > right, but looping in an aio function seemed not so trivial to me. > it seems more and more obvious to me that the best would be to change > all the remaining aio routines to co routines. The pattern for AIO functions is that the real work of submitting requests is done in the AIO callback, and it submits new AIO requests calling back into the same callback as long as acb->remaining_secs > 0 (or something like that). You can still see that kind of thing alive in qed_aio_next_io(), (most of?) the rest is converted to coroutines because it makes the code look nicer. > in this case i could add the too big logic in iscsi_co_discard and simply call > it from iscsi_co_write_zeroes. I think that would be the nicest solution. Kevin
Am 10.07.2013 16:28, schrieb Kevin Wolf: > Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben: >> Am 10.07.2013 13:33, schrieb Kevin Wolf: >>> Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: >>>> if the target does not support UNMAP or the request >>>> is too big silently ignore the discard request. >>>> >>>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> Why not loop for the "too big" case? You can probably use the same logic >>> for unmapping the whole device in .bdrv_create and here. >> right, but looping in an aio function seemed not so trivial to me. >> it seems more and more obvious to me that the best would be to change >> all the remaining aio routines to co routines. > The pattern for AIO functions is that the real work of submitting > requests is done in the AIO callback, and it submits new AIO requests > calling back into the same callback as long as acb->remaining_secs > 0 > (or something like that). > > You can still see that kind of thing alive in qed_aio_next_io(), (most > of?) the rest is converted to coroutines because it makes the code look > nicer. would you agree if I leave the easy version in just to fix the potential problems if iscsi_aio_discard is called with too high nb_sectors or on a storage where UNMAP is unsupported. I will add a TODO with the comment that the limit of iscsi->max_unmap should be replaced by a loop once the routine is replaced by a coroutine? > >> in this case i could add the too big logic in iscsi_co_discard and simply call >> it from iscsi_co_write_zeroes. > I think that would be the nicest solution. I promised to take care of this for 1.7.0 latest. Peter
Am 10.07.2013 um 16:49 hat Peter Lieven geschrieben: > Am 10.07.2013 16:28, schrieb Kevin Wolf: > > Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben: > >> Am 10.07.2013 13:33, schrieb Kevin Wolf: > >>> Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: > >>>> if the target does not support UNMAP or the request > >>>> is too big silently ignore the discard request. > >>>> > >>>> Signed-off-by: Peter Lieven <pl@kamp.de> > >>> Why not loop for the "too big" case? You can probably use the same logic > >>> for unmapping the whole device in .bdrv_create and here. > >> right, but looping in an aio function seemed not so trivial to me. > >> it seems more and more obvious to me that the best would be to change > >> all the remaining aio routines to co routines. > > The pattern for AIO functions is that the real work of submitting > > requests is done in the AIO callback, and it submits new AIO requests > > calling back into the same callback as long as acb->remaining_secs > 0 > > (or something like that). > > > > You can still see that kind of thing alive in qed_aio_next_io(), (most > > of?) the rest is converted to coroutines because it makes the code look > > nicer. > would you agree if I leave the easy version in just to fix the potential > problems if iscsi_aio_discard is called with too high nb_sectors or > on a storage where UNMAP is unsupported. > > I will add a TODO with the comment that the limit of iscsi->max_unmap should > be replaced by a loop once the routine is replaced by a coroutine? Meh, another pony I don't get... ;-) Leaving a TODO comment for now is okay with me. > >> in this case i could add the too big logic in iscsi_co_discard and simply call > >> it from iscsi_co_write_zeroes. > > I think that would be the nicest solution. > I promised to take care of this for 1.7.0 latest. Okay, thanks. Kevin
Am 10.07.2013 16:58, schrieb Kevin Wolf: > Am 10.07.2013 um 16:49 hat Peter Lieven geschrieben: >> Am 10.07.2013 16:28, schrieb Kevin Wolf: >>> Am 10.07.2013 um 16:04 hat Peter Lieven geschrieben: >>>> Am 10.07.2013 13:33, schrieb Kevin Wolf: >>>>> Am 27.06.2013 um 15:11 hat Peter Lieven geschrieben: >>>>>> if the target does not support UNMAP or the request >>>>>> is too big silently ignore the discard request. >>>>>> >>>>>> Signed-off-by: Peter Lieven <pl@kamp.de> >>>>> Why not loop for the "too big" case? You can probably use the same logic >>>>> for unmapping the whole device in .bdrv_create and here. >>>> right, but looping in an aio function seemed not so trivial to me. >>>> it seems more and more obvious to me that the best would be to change >>>> all the remaining aio routines to co routines. >>> The pattern for AIO functions is that the real work of submitting >>> requests is done in the AIO callback, and it submits new AIO requests >>> calling back into the same callback as long as acb->remaining_secs > 0 >>> (or something like that). >>> >>> You can still see that kind of thing alive in qed_aio_next_io(), (most >>> of?) the rest is converted to coroutines because it makes the code look >>> nicer. >> would you agree if I leave the easy version in just to fix the potential >> problems if iscsi_aio_discard is called with too high nb_sectors or >> on a storage where UNMAP is unsupported. >> >> I will add a TODO with the comment that the limit of iscsi->max_unmap should >> be replaced by a loop once the routine is replaced by a coroutine? > Meh, another pony I don't get... ;-) > > Leaving a TODO comment for now is okay with me. forget about the TODO. Looking at what I have already got, implementing iscsi_co_discard is low hanging fruit. I will implement it directly. With the too big loop and all the benefits of the co routines. Peter
diff --git a/block/iscsi.c b/block/iscsi.c index e9ecfce..0567b46 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -694,6 +694,14 @@ iscsi_aio_discard(BlockDriverState *bs, acb->sector_num = sector_num; acb->retries = ISCSI_CMD_RETRIES; + if (!iscsilun->lbpu || + sector_qemu2lun(nb_sectors, iscsilun) > iscsilun->max_unmap) { + /* silently ignore discard request */ + acb->status = 0; + iscsi_schedule_bh(acb); + return &acb->common; + } + if (iscsi_aio_discard_acb(acb) != 0) { qemu_aio_release(acb); return NULL;
if the target does not support UNMAP or the request is too big silently ignore the discard request. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/iscsi.c | 8 ++++++++ 1 file changed, 8 insertions(+)