On Wed, Apr 17, 2013 at 01:07:56PM +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 17, 2013 at 09:29:53AM +0800, Asias He wrote:
> > On Tue, Apr 16, 2013 at 08:58:27PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 16, 2013 at 05:16:51PM +0800, Asias He wrote:
> > > > This patch makes vhost_scsi_flush() wait for all the pending requests
> > > > issued before the flush operation to be finished.
> > > >
> > > > Changes in v5:
> > > > - Use kref and completion
> > > > - Fail req if vs->vs_inflight is NULL
> > > > - Rename tcm_vhost_alloc_inflight to tcm_vhost_set_inflight
> > > >
> > > > Changes in v4:
> > > > - Introduce vhost_scsi_inflight
> > > > - Drop array to track flush
> > > > - Use RCU to protect vs_inflight explicitly
> > > >
> > > > Changes in v3:
> > > > - Rebase
> > > > - Drop 'tcm_vhost: Wait for pending requests in
> > > > vhost_scsi_clear_endpoint()' in this series, we already did that in
> > > > 'tcm_vhost: Use vq->private_data to indicate if the endpoint is setup'
> > > >
> > > > Changes in v2:
> > > > - Increase/Decrease inflight requests in
> > > > vhost_scsi_{allocate,free}_cmd and tcm_vhost_{allocate,free}_evt
> > > >
> > > > Signed-off-by: Asias He <asias@xxxxxxxxxx>
> > >
> > > OK looks good, except error handling needs to be fixed.
>
> Let me be more explicit here. There are two rules that this violates:
> an ioctl should either succeed and return code >= 0,
> or fail and return code < 0. In later case it should not
> have any effect.
> This patch instead leaves the device in a bad state
> on an OOM error.
>
> There should be 1 way to test started/stopped state,
> and that is backend != NULL.
>
>
> So how to fix this? As I said, split up tcm_vhost_set_inflight.
> Have tcm_vhost_init_inflight instead, to simply init counters
> without alloc and the RCU things. Now in set features ioctl,
> Do allocations that can fail before you change state.
> Now set features and after a barrier set inflight.
Do you want do all this in other places where vhost_scsi_flush is
called? It's a pain.
>
> > > > ---
> > > > drivers/vhost/tcm_vhost.c | 101 +++++++++++++++++++++++++++++++++++++++++++---
> > > > drivers/vhost/tcm_vhost.h | 5 +++
> > > > 2 files changed, 101 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > > > index 4ae6725..ef40a8f 100644
> > > > --- a/drivers/vhost/tcm_vhost.c
> > > > +++ b/drivers/vhost/tcm_vhost.c
> > > > @@ -74,6 +74,11 @@ enum {
> > > > #define VHOST_SCSI_MAX_VQ 128
> > > > #define VHOST_SCSI_MAX_EVENT 128
> > > >
> > > > +struct vhost_scsi_inflight {
> > > > + struct completion comp; /* Wait for the flush operation to finish */
> > > > + struct kref kref; /* Refcount for the inflight reqs */
> > > > +};
> > > > +
> > > > struct vhost_scsi {
> > > > /* Protected by vhost_scsi->dev.mutex */
> > > > struct tcm_vhost_tpg **vs_tpg;
> > > > @@ -91,6 +96,8 @@ struct vhost_scsi {
> > > > struct mutex vs_events_lock; /* protect vs_events_dropped,events_nr */
> > > > bool vs_events_dropped; /* any missed events */
> > > > int vs_events_nr; /* num of pending events */
> > > > +
> > > > + struct vhost_scsi_inflight __rcu *vs_inflight; /* track inflight reqs */
> > > > };
> > > >
> > > > /* Local pointer to allocated TCM configfs fabric module */
> > > > @@ -108,6 +115,51 @@ static int iov_num_pages(struct iovec *iov)
> > > > ((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
> > > > }
> > > >
> > > > +static int tcm_vhost_set_inflight(struct vhost_scsi *vs)
> > > > +{
> > > > + struct vhost_scsi_inflight *inflight;
> > > > + int ret = -ENOMEM;
> > > > +
> > > > + inflight = kzalloc(sizeof(*inflight), GFP_KERNEL);
> > >
> > > kzalloc is not needed, you initialize all fields.
> >
> > okay.
> >
> > > > + if (inflight) {
> > > > + kref_init(&inflight->kref);
> > > > + init_completion(&inflight->comp);
> > > > + ret = 0;
> > > > + }
> > > > + rcu_assign_pointer(vs->vs_inflight, inflight);
> > >
> > > So if allocation fails, we stop tracking inflights?
> > >
> > > This looks strange, and could break guests. Why not the usual
> > > if (!inflight)
> > > return -ENOMEM;
> >
> > If allocation fails, we abort further reqs.
> > No need to track.
> > Why it will break guest and how?
>
> Well if not commands succeed presumably storage does not work?
> Also need to add a comment about this hack.
>
> But isn't failing the ioctl cleaner?
> There will be less code on data path and less tricks.
The point is what will you do when allocation fails in flush. You can
not do it in a cleaner way.
> > > > + synchronize_rcu();
> > >
> > > open call is different:
> > > - sync is not needed
> > > - should use RCU_INIT_POINTER and not rcu_assign_pointer
> > >
> > > So please move these out and make this function return the struct:
> > > struct vhost_scsi_inflight *inflight
> > > tcm_vhost_alloc_inflight(void)
> >
> > synchronize_rcu is actually needed.
>
> It's not needed for open: there's no old inflight there.
That's true. We can move it out. But it has no effect in open.
> > tcm_vhost_inc_inflight
> > {
> >
> > rcu_read_lock();
> > inflight = rcu_dereference(vs->vs_inflight);
> >
> > /*
> > * Possible race window here:
> > * if inflight points to old inflight and
> > * wait_for_completion runs before we call kref_get here,
> > * We may free the old inflight
> > * however, there is still one in flight which should be
> > * tracked by the old inflight.
> > */
> >
> > kref_get(&inflight->kref);
> > rcu_read_unlock();
> >
> > return inflight;
> > }
>
>
> By the way I see a bug. vhost_scsi_set_features does smp_wmb
> and that is not paired with anything. I think we need to pair
> it with an rmb before get_inflight and add a comment
> about pairing in both places.
Let's fix it in other patches.
> > >
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static struct vhost_scsi_inflight *
> > > > +tcm_vhost_inc_inflight(struct vhost_scsi *vs)
> > >
> > > And then inc will not need to return inflight pointer,
> > > which is really unusual.
> >
> > No you still need to return inflight. You need it for each tcm_vhost_cmd or
> > tcm_vhost_evt.
>
> That's true. But it's a strange thing to do on increment.
> Please rename inc to get and dec to put to make this
> clear.
So we still need it. Okay, let's do the rename.
> > > > +{
> > > > + struct vhost_scsi_inflight *inflight;
> > > > +
> > > > + rcu_read_lock();
> > > > + inflight = rcu_dereference(vs->vs_inflight);
> > > > + if (inflight)
> > > > + kref_get(&inflight->kref);
> > > > + rcu_read_unlock();
> > > > +
> > > > + return inflight;
> > > > +}
> > > > +
> > > > +void tcm_vhost_done_inflight(struct kref *kref)
> > > > +{
> > > > + struct vhost_scsi_inflight *inflight;
> > > > +
> > > > + inflight = container_of(kref, struct vhost_scsi_inflight, kref);
> > > > + complete(&inflight->comp);
> > > > +}
> > > > +
> > > > +static void tcm_vhost_dec_inflight(struct vhost_scsi_inflight *inflight)
> > > > +{
> > > > + if (inflight)
> > >
> > > Here as in other places, inflight must never be NULL.
> > > Pls fix code so that invariant holds.
> > >
> > > > + kref_put(&inflight->kref, tcm_vhost_done_inflight);
> > > > +}
> > > > +
> > > > static bool tcm_vhost_check_feature(struct vhost_scsi *vs, int feature)
> > > > {
> > > > bool ret = false;
> > > > @@ -402,6 +454,7 @@ static int tcm_vhost_queue_tm_rsp(struct se_cmd *se_cmd)
> > > > static void tcm_vhost_free_evt(struct vhost_scsi *vs, struct tcm_vhost_evt *evt)
> > > > {
> > > > mutex_lock(&vs->vs_events_lock);
> > > > + tcm_vhost_dec_inflight(evt->inflight);
> > > > vs->vs_events_nr--;
> > > > kfree(evt);
> > > > mutex_unlock(&vs->vs_events_lock);
> > > > @@ -413,21 +466,27 @@ static struct tcm_vhost_evt *tcm_vhost_allocate_evt(struct vhost_scsi *vs,
> > > > struct tcm_vhost_evt *evt;
> > > >
> > > > mutex_lock(&vs->vs_events_lock);
> > > > - if (vs->vs_events_nr > VHOST_SCSI_MAX_EVENT) {
> > > > - vs->vs_events_dropped = true;
> > > > - mutex_unlock(&vs->vs_events_lock);
> > > > - return NULL;
> > > > - }
> > > > + if (vs->vs_events_nr > VHOST_SCSI_MAX_EVENT)
> > > > + goto out;
> > > >
> > > > evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> > >
> > > BTW it looks like we should replace this kzalloc with kmalloc.
> > > Should be a separate patch ...
> >
> > This belongs to the hotplug series. I will add to it.
> >
> > > > if (evt) {
> > > > evt->event.event = event;
> > > > evt->event.reason = reason;
> > > > + evt->inflight = tcm_vhost_inc_inflight(vs);
> > > > + if (!evt->inflight) {
> > >
> > > We drop an event because earlier
> > > we run out of memory for allocating the inflight counter.
> > > Does not make sense to me.
> >
> > This tries to abort further reqs if we fail to allocate inflight.
>
> Yes, it prevents a crash, but it's better to tell the user that
> something's wrong.
So crash it?
> > > > + kfree(evt);
> > > > + goto out;
> > > > + }
> > > > vs->vs_events_nr++;
> > > > }
> > > > mutex_unlock(&vs->vs_events_lock);
> > > >
> > > > return evt;
> > > > +out:
> > > > + vs->vs_events_dropped = true;
> > > > + mutex_unlock(&vs->vs_events_lock);
> > > > + return NULL;
> > > > }
> > > >
> > > > static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> > > > @@ -445,6 +504,8 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
> > > > kfree(tv_cmd->tvc_sgl);
> > > > }
> > > >
> > > > + tcm_vhost_dec_inflight(tv_cmd->inflight);
> > > > +
> > > > kfree(tv_cmd);
> > > > }
> > > >
> > > > @@ -595,6 +656,9 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
> > > > tv_cmd->tvc_data_direction = data_direction;
> > > > tv_cmd->tvc_nexus = tv_nexus;
> > > > tv_cmd->tvc_vhost = vs;
> > > > + tv_cmd->inflight = tcm_vhost_inc_inflight(vs);
> > > > + if (!tv_cmd->inflight)
> > > > + return ERR_PTR(-ENOMEM);
> > > >
> > > > return tv_cmd;
> > > > }
> > > > @@ -982,12 +1046,35 @@ static void vhost_scsi_flush_vq(struct vhost_scsi *vs, int index)
> > > >
> > > > static void vhost_scsi_flush(struct vhost_scsi *vs)
> > > > {
> > > > + struct vhost_scsi_inflight *inflight;
> > > > int i;
> > > >
> > > > + /* inflight points to the old inflight */
> > > > + inflight = rcu_dereference_protected(vs->vs_inflight,
> > > > + lockdep_is_held(&vs->dev.mutex));
> > > > +
> > > > + /* Allocate a new inflight and make vs->vs_inflight points to it */
> > > > + if (tcm_vhost_set_inflight(vs) < 0)
> > > > + pr_warn("vhost_scsi_flush failed to allocate inflight\n");
> > >
> > > That's unlikely to reach the application. How about we stop here,
> > > and propagate the error to ioctl caller?
> >
> > What user application can do in this case. Especially, the vhost_scsi_flush
> > fails when user tries to shutdown the guest.
>
> I am not sure why you walk about close. Only thing calling flush
> at the moment is set features ioctl.
Which code are you looking at? Try
git show linus/master:drivers/vhost/tcm_vhost.c
Flush is called in vhost_scsi_set_endpoint and vhost_scsi_clear_endpoint
as well.
> But let's assume you have another patch that calls flush
> on close. Then this is a bug: close should not try to allocate
> memory. On close, we really should just set private data to NULL.
> Then we know no new requests will be submitted.
> No need to change inflight at all.
Well, do you need vhost_scsi_clear_endpoint on close?
Who wanted to allocate memory in flush at the first palce?
> > What we are doing now is safer than just stops here. Because
> > 1) we still flush all the existing reqs
> > 2) Further reqs will be aborted.
>
>
> Stop really should stop processing new requests, not cancel them. If
> you cancel on close, we will not be able to stop kernel and then restart
> without errors.
What's the difference of 'stop processing new requests' and 'cancel them'?
On close, the vq->private_data is set to NULL. No new reqs will be
queued. So we can stop it.
> > > > +
> > > > + /*
> > > > + * The inflight->kref was initialized to 1. We decrement it here to
> > > > + * indicate the start of the flush operation so that it will reach 0
> > > > + * when all the reqs are finished.
> > > > + */
> > > > + kref_put(&inflight->kref, tcm_vhost_done_inflight);
> > > > +
> > > > + /* Flush both the vhost poll and vhost work */
> > > > for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
> > > > vhost_scsi_flush_vq(vs, i);
> > > > vhost_work_flush(&vs->dev, &vs->vs_completion_work);
> > > > vhost_work_flush(&vs->dev, &vs->vs_event_work);
> > > > +
> > > > + /* Wait for all reqs issued before the flush to be finished */
> > > > + if (inflight) {
> > >
> > > inflight should never be NULL, otherwise inflight
> > > tracjing is not effective. Please fix error handling so we
> > > never reach here with inflight == NULL.
> >
> > It is effective. We abort the req if we can not track it.
>
> Yes. But that's guest visible, isn't it?
What else the host does not know?
> > > > + wait_for_completion(&inflight->comp);
> > > > + kfree(inflight);
> > > > + }
> > > > }
> > > >
> > > > /*
> > > > @@ -1196,6 +1283,9 @@ static int vhost_scsi_open(struct inode *inode, struct file *f)
> > > > s->vs_events_dropped = false;
> > > > mutex_init(&s->vs_events_lock);
> > > >
> > > > + if (tcm_vhost_set_inflight(s) < 0)
> > > > + return -ENOMEM;
> > > > +
> > >
> > > Better propagate the return code to user.
> >
> > We are returning -ENOMEM to user, no?
>
> I mean if you call a function that returns 0 or errno,
> you should do
> r = tcm_vhost_set_inflight
> if (r)
> return r;
>
> then it's easier to add more error codes in
> tcm_vhost_set_inflight if needed.
This is true, but what error code do you want to add?
> However this was a general comment, I think tcm_vhost_set_inflight
> should be split up so it won't return int.
What do you really want?
> > > > s->vqs[VHOST_SCSI_VQ_CTL].handle_kick = vhost_scsi_ctl_handle_kick;
> > > > s->vqs[VHOST_SCSI_VQ_EVT].handle_kick = vhost_scsi_evt_handle_kick;
> > > > for (i = VHOST_SCSI_VQ_IO; i < VHOST_SCSI_MAX_VQ; i++)
> > > > @@ -1221,6 +1311,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f)
> > > > vhost_scsi_clear_endpoint(s, &t);
> > > > vhost_dev_stop(&s->dev);
> > > > vhost_dev_cleanup(&s->dev, false);
> > > > + kfree(s->vs_inflight);
> > > > kfree(s);
> > > > return 0;
> > > > }
> > > > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > > > index 94e9ee53..7567767 100644
> > > > --- a/drivers/vhost/tcm_vhost.h
> > > > +++ b/drivers/vhost/tcm_vhost.h
> > > > @@ -2,6 +2,7 @@
> > > > #define TCM_VHOST_NAMELEN 256
> > > > #define TCM_VHOST_MAX_CDB_SIZE 32
> > > >
> > > > +struct vhost_scsi_inflight;
> > > > struct tcm_vhost_cmd {
> > > > /* Descriptor from vhost_get_vq_desc() for virt_queue segment */
> > > > int tvc_vq_desc;
> > > > @@ -37,6 +38,8 @@ struct tcm_vhost_cmd {
> > > > unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
> > > > /* Completed commands list, serviced from vhost worker thread */
> > > > struct llist_node tvc_completion_list;
> > > > + /* Used to track inflight req */
> > > > + struct vhost_scsi_inflight *inflight;
> > > > };
> > > >
> > > > struct tcm_vhost_nexus {
> > > > @@ -91,6 +94,8 @@ struct tcm_vhost_evt {
> > > > struct virtio_scsi_event event;
> > > > /* virtio_scsi event list, serviced from vhost worker thread */
> > > > struct llist_node list;
> > > > + /* Used to track inflight req */
> > > > + struct vhost_scsi_inflight *inflight;
> > > > };
> > > >
> > > > /*
> > > > --
> > > > 1.8.1.4
> >
> > --
> > Asias
--
Asias
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
