Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

...

> > > +    list = records;
> > > +    extents = g_new0(CXLDCExtentRaw, num_extents);
> > > +    while (list) {
> > > +        CXLDCExtent *ent;
> > > +        bool skip_extent = false;
> > > +
> > > +        offset = list->value->offset;
> > > +        len = list->value->len;
> > > +
> > > +        extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > +        extents[i].len = len;
> > > +        memset(extents[i].tag, 0, 0x10);
> > > +        extents[i].shared_seq = 0;
> > > +
> > > +        if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > +            type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > +            /*
> > > +             *  if the extent is still pending to be added to the host,  
> > 
> > Odd spacing.
> >   
> > > +             * remove it from the pending extent list, so later when the add
> > > +             * response for the extent arrives, the device can reject the
> > > +             * extent as it is not in the pending list.
> > > +             */
> > > +            ent = cxl_dc_extent_exists(&dcd->dc.extents_pending_to_add,
> > > +                        &extents[i]);
> > > +            if (ent) {
> > > +                QTAILQ_REMOVE(&dcd->dc.extents_pending_to_add, ent, node);
> > > +                g_free(ent);
> > > +                skip_extent = true;
> > > +            } else if (!cxl_dc_extent_exists(&dcd->dc.extents, &extents[i])) {
> > > +                /* If the exact extent is not in the accepted list, skip */
> > > +                skip_extent = true;
> > > +            }  
> > I think we need to reject case of some extents skipped and others not.
> > That's not supported yet so we need to complain if we get it at least. Maybe we need
> > to do two passes so we know this has happened early (or perhaps this is a later
> > patch in which case a todo here would help).  
> 
> Skip here does not mean the extent is invalid, it just means the extent
> is still pending to add, so remove them from pending list would be
> enough to reject the extent, no need to release further. That is based
> on your feedback on v4.

Ah. I'd missunderstood.

> 
> The loop here is only to collect the extents to sent to the event log. 
> But as you said, we need one pass before updating pending list.
> Actually if we do not allow the above case where extents to release is
> still in the pending to add list, we can just return here with error, no
> extra dry run needed. 
> 
> What do you think?

I think we need a way to back out extents from the pending to add list
so we can create the race where they are offered to the OS and it takes
forever to accept and by the time it does we've removed them.

> 
> >   
> > > +        
> > > +
> > > +        /* No duplicate or overlapped extents are allowed */
> > > +        if (test_any_bits_set(blk_bitmap, offset / block_size,
> > > +                              len / block_size)) {
> > > +            error_setg(errp, "duplicate or overlapped extents are detected");
> > > +            return;
> > > +        }
> > > +        bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> > > +
> > > +        list = list->next;
> > > +        if (!skip_extent) {
> > > +            i++;  
> > Problem is if we skip one in the middle the records will be wrong below.  
> 
> Why? Only extents passed the check will be stored in variable extents and
> processed further and i be updated. 
> For skipped ones, since i is not updated, they will be
> overwritten by following valid ones.
Ah. I'd missed the fact you store into the extent without a check on validity
but only move the index on if they were valid. Then rely on not passing a trailing
entry at the end.
If would be more readable I think if local variables were used for the parameters
until we've decided not to skip and the this ended with

        if (!skip_extent) {
            extents[i] = (DCXLDCExtentRaw) {
                .start_dpa = ...
	        ...
            };
            i++
        }
We have local len already so probably just need
uint64_t start_dpa = offset + dcd->dc.regions[rid].base;

Also maybe skip_extent_evlog or something like that to explain we are only
skipping that part. 
Helps people like me who read it completely wrong!

Jonathan