Re: [PATCH v4 3/6] block-copy: improve comments of BlockCopyTask and BlockCopyState types and functions

[Vladimir Sementsov-Ogievskiy]

21.06.2021 11:13, Emanuele Giuseppe Esposito wrote:
> On 19/06/2021 20:31, Vladimir Sementsov-Ogievskiy wrote:
> > 19.06.2021 18:23, Vladimir Sementsov-Ogievskiy wrote:
> > > >   typedef struct BlockCopyTask {
> > > >       AioTask task;
> > > > +    /*
> > > > +     * IN parameters. Initialized in block_copy_task_create()
> > > > +     * and never changed.
> > > > +     */
> > >
> > > That's just not true for method field :(
> >
> > I think, we just need to document that @method is never accessed concurrently
> Ok I got confused in the last patch. Method is read by block_copy_task_entry 
> only after it is re-set in block_copy_dirty_clusters loop. Sorry for that.
>
> Will leave it as IN and document it better.

IN/State/OUT doesn't really help IMHO. As most of fields of interest doesn't 
cleanly belong to any of the categories. Better documentation is good. If same 
variables share the same access documentation - make them a group.

> Still, moving the lock granularity inside the while loop might not be too bad. 
> Not sure though. At this point skip_unallocated can also be an atomic, even 
> though I sense that you won't like that :)

As I said in parallel email, We can't keep lock locked during block_status. So, 
make it atomic if it is convenient.


--
Best regards,
Vladimir