Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] virtio-mem: swallowed memmap unplug errors in virtio_mem_sbm_try_remove_unplugged_mb() --
commit b0988c80262195af51a910d0ad85c26ee566fe77 Author: Nikolaos Karaolidis <[email protected]> virtio-mem: support memmap_on_memory in Sub Block Mode This commit adds memmap_on_memory support for Sub Block Mode in virtio-mem when the self-hosted memmap spans complete subblocks. It keeps the memmap subblocks plugged while a memory block is added to Linux, skips them when unplugging from an added block, and unplugs them once the block is removed. > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c > index 688c178c760f5..d6623f67b76bb 100644 > --- a/drivers/virtio/virtio_mem.c > +++ b/drivers/virtio/virtio_mem.c [ ... ] > @@ -1617,35 +1628,65 @@ static int virtio_mem_sbm_unplug_mb(struct virtio_mem > *vm, unsigned long mb_id) > { > uint64_t nb_sb = vm->sbm.sbs_per_mb; > > - return virtio_mem_sbm_unplug_any_sb_raw(vm, mb_id, &nb_sb); > + return virtio_mem_sbm_unplug_any_sb_raw(vm, mb_id, 0, &nb_sb); > +} > + > +/* > + * Unplug the memmap subblocks of a memory block removed from Linux and mark > it > + * unused. The memmap subblocks are deducted from @nb_sb (clamped), even if > + * unplugging them fails. On error, the memory block stays > + * VIRTIO_MEM_SBM_MB_PLUGGED and virtio_mem_cleanup_pending_mb() retries. > + * > + * Will modify the state of the memory block. > + */ > +static int virtio_mem_sbm_finish_remove_mb(struct virtio_mem *vm, > + unsigned long mb_id, uint64_t *nb_sb) > +{ > + const int count = vm->sbm.memmap_sbs; > + int rc; > + > + if (count) { > + if (nb_sb) > + *nb_sb -= min_t(uint64_t, *nb_sb, count); > + virtio_mem_sbm_set_mb_state(vm, mb_id, > + VIRTIO_MEM_SBM_MB_PLUGGED); > + rc = virtio_mem_sbm_unplug_sb(vm, mb_id, 0, count); > + if (rc) > + return rc; > + } > + virtio_mem_sbm_set_mb_state(vm, mb_id, VIRTIO_MEM_SBM_MB_UNUSED); > + return 0; > } > > /* > - * Try (offlining and) removing memory from Linux in case all subblocks are > - * unplugged. Can be called on online and offline memory blocks. > + * Try (offlining and) removing memory from Linux in case all subblocks > except > + * the memmap subblocks are unplugged. Can be called on online and offline > + * memory blocks. > * > * May modify the state of memory blocks in virtio-mem. > */ > static int virtio_mem_sbm_try_remove_unplugged_mb(struct virtio_mem *vm, > - unsigned long mb_id) > + unsigned long mb_id, > + uint64_t *nb_sb) > { > + const int first_sb = vm->sbm.memmap_sbs; > int rc; > > /* > * Once all subblocks of a memory block were unplugged, offline and > - * remove it. > + * remove it. The memmap subblocks alone provide no memory to Linux. > */ > - if (!virtio_mem_sbm_test_sb_unplugged(vm, mb_id, 0, vm->sbm.sbs_per_mb)) > + if (!virtio_mem_sbm_test_sb_unplugged(vm, mb_id, first_sb, > + vm->sbm.sbs_per_mb - first_sb)) > return 0; > > /* offline_and_remove_memory() works for online and offline memory. */ > mutex_unlock(&vm->hotplug_mutex); > rc = virtio_mem_sbm_offline_and_remove_mb(vm, mb_id); > mutex_lock(&vm->hotplug_mutex); > - if (!rc) > - virtio_mem_sbm_set_mb_state(vm, mb_id, > - VIRTIO_MEM_SBM_MB_UNUSED); > - return rc; > + if (rc) > + return rc; > + return virtio_mem_sbm_finish_remove_mb(vm, mb_id, nb_sb); > } [ ... ] > @@ -2072,7 +2130,7 @@ static int virtio_mem_sbm_unplug_any_sb_online(struct > virtio_mem *vm, > } > > unplugged: > - rc = virtio_mem_sbm_try_remove_unplugged_mb(vm, mb_id); > + rc = virtio_mem_sbm_try_remove_unplugged_mb(vm, mb_id, nb_sb); > if (rc) > vm->sbm.have_unplugged_mb = 1; > /* Ignore errors, this is not critical. We'll retry later. */ > @@ -2389,11 +2447,11 @@ static int virtio_mem_cleanup_pending_mb(struct > virtio_mem *vm) > > mutex_lock(&vm->hotplug_mutex); > virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_MOVABLE_PARTIAL) > - rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id); > + rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL); > virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_KERNEL_PARTIAL) > - rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id); > + rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL); > virtio_mem_sbm_for_each_mb(vm, id, VIRTIO_MEM_SBM_MB_OFFLINE_PARTIAL) > - rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id); > + rc |= virtio_mem_sbm_try_remove_unplugged_mb(vm, id, NULL); > mutex_unlock(&vm->hotplug_mutex); > > if (rc) [Severity: Medium] When virtio_mem_sbm_offline_and_remove_mb() succeeds in virtio_mem_sbm_try_remove_unplugged_mb(), it calls virtio_mem_sbm_finish_remove_mb() to transition the memory block to VIRTIO_MEM_SBM_MB_PLUGGED and unplug the memmap subblocks via virtio_mem_sbm_unplug_sb(). Before this change, virtio_mem_sbm_try_remove_unplugged_mb() only failed when virtio_mem_sbm_offline_and_remove_mb() failed, leaving the block added to Linux in a *_PARTIAL state with all subblocks already unplugged from the device. Both callers (virtio_mem_sbm_unplug_any_sb_online() and virtio_mem_cleanup_pending_mb()) therefore set vm->sbm.have_unplugged_mb and return 0 to ignore the offlining failure. With this change, if virtio_mem_sbm_finish_remove_mb() fails to unplug the memmap subblocks (for example, returning -EAGAIN on a concurrent config change during offlining, -ETXTBSY on VIRTIO_MEM_RESP_BUSY, or -EINVAL on VIRTIO_MEM_RESP_ERROR), both callers treat that device unplug error the same way as an offlining failure: 1. They set vm->sbm.have_unplugged_mb even though the memory block was already removed from Linux and moved to VIRTIO_MEM_SBM_MB_PLUGGED rather than a *_PARTIAL state. 2. In virtio_mem_cleanup_pending_mb(), the VIRTIO_MEM_SBM_MB_PLUGGED loop has already run earlier in the function: virtio_mem_run_wq() virtio_mem_cleanup_pending_mb() virtio_mem_sbm_for_each_mb(..., VIRTIO_MEM_SBM_MB_PLUGGED) ... virtio_mem_sbm_for_each_mb(..., VIRTIO_MEM_SBM_MB_*_PARTIAL) virtio_mem_sbm_try_remove_unplugged_mb() virtio_mem_sbm_finish_remove_mb() -> fails, sets MB_PLUGGED returns 0 virtio_mem_unplug_request() / virtio_mem_plug_request() Because virtio_mem_cleanup_pending_mb() swallows the error and returns 0, virtio_mem_run_wq() proceeds directly to virtio_mem_unplug_request() or virtio_mem_plug_request() in the same workqueue run while the failed block's memmap subblocks are still counted in vm->plugged_size. Doesn't this cause virtio_mem_unplug_request() to unplug extra subblocks from another active memory block (or virtio_mem_plug_request() to under-plug)? 3. In both callers, device errors from virtio_mem_sbm_finish_remove_mb() are masked instead of aborting the operation and propagating to virtio_mem_run_wq() (unlike virtio_mem_sbm_unplug_any_sb_offline(), which returns the error directly). For instance, in virtio_mem_sbm_unplug_any_sb_online(), returning 0 allows virtio_mem_sbm_unplug_request() to keep unplugging subsequent blocks on the device error, and in virtio_mem_run_wq(), -EAGAIN will not jump to retry and a fatal -EINVAL from VIRTIO_MEM_RESP_ERROR will be turned into -EBUSY via have_unplugged_mb instead of setting vm->broken = true. Should virtio_mem_sbm_try_remove_unplugged_mb() distinguish an offlining failure in virtio_mem_sbm_offline_and_remove_mb() (setting vm->sbm.have_unplugged_mb and returning 0) from a device unplug failure in virtio_mem_sbm_finish_remove_mb() (propagating the error code to the caller)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
