[lustre-devel] [PATCH 03/10] lustre: lnd: rework map_on_demand behavior

[James Simmons]

> On Sun, Oct 14 2018, James Simmons wrote:
> 
> > @@ -1320,9 +1324,8 @@ static void kiblnd_destroy_fmr_pool(struct kib_fmr_pool *fpo)
> >  {
> >  	LASSERT(!fpo->fpo_map_count);
> >  
> > -	if (fpo->fpo_is_fmr) {
> > -		if (fpo->fmr.fpo_fmr_pool)
> > -			ib_destroy_fmr_pool(fpo->fmr.fpo_fmr_pool);
> > +	if (fpo->fpo_is_fmr && fpo->fmr.fpo_fmr_pool) {
> > +		ib_destroy_fmr_pool(fpo->fmr.fpo_fmr_pool);
> >  	} else {
> >  		struct kib_fast_reg_descriptor *frd;
> >  		int i = 0;
> 
> This change looks strange.
> With the new code: if fpo_fmr_pool is NULL, then the fpo_pool_list is
> destroyed, even though fpo_is_fmr.
> 
> Are we sure that is correct??
> I don't really understand this code so it might be correct, but the
> change looks wrong.
> 
> Can someone please confirm that it really is right - and maybe explain
> why?

It is wrong!!! The only reason it didn't show up is that fp_fmr_pool is
very unlikely to NULL. I understand why this bug was introduced since
its desirable to reduce the indentation. This is the fault of how the
code is written. To avoid this in the future it should be written:

if (!fpo->fpo_is_fmr) {
	struct kib_fast_reg_descriptor *frd, *tmp;
	....
} else if (fpo->fmr.fpo_fmr_pool) {
	ib_destroy_fmr_pool(fpo->fmr.fpo_fmr_pool);
}

I opened a ticket for https://jira.whamcloud.com/browse/LU-11552 and 
submitted a patch - https://review.whamcloud.com/#/c/33408. To address
this.

So please remove this part from the change. We could redo the patch to
include the above or I could just send a follow up patch to make this
code clearer. I leave that up to you.