phdeniel/nfs-ganesha — comment on commit df4d1a1ab0809769c7e125c65e7cfe6dd84ff6ae
File:  line 0
By mmakc:

Bonjour Philippe,

PD> I have time (at least) to check your commits. I do have a few questions:

Thanks for looking.

PD> - in cache_inode_get.c : why removing the unlock in the case
PD> cache_inode_valid fails ?

The code just looked "wrong".

There is no indication that cache_inode_valid() takes extra locks in
case of failure (actually, it cannot do that of it will always
deadlock), so this code looked like it was trying to unlock the entry
twice on failure.

PD> The entry will remain locked which will lead to later
PD> deadlocks. The fact is that this function almost never returns
PD> an error (which hides many things) : your code will result in a
PD> segfault because you set pentry to NULL then deference
PD> pentry->lock

Note that this isn't my change - I did not add pentry changes,
I've only removed the lock. In the hindsight I should've spotted this.

I think this code was meant to say:

P_w(&pentry->lock);
if ((*pstatus = cache_inode_valid(....)) != CACHE_INODE_SUCCESS)
{
V_w(&pentry->lock);
pentry = NULL;
}
else
{
V_w(&pentry->lock);
}

The 'else' got lost and this created the confusion.

In fact, I usually write fragments like this slightly differently:

P_w(&pentry->lock);
*pstatus = cache_inode_valid(....);
V_w(&pentry->lock);

if (*pstatus != CACHE_INODE_SUCCESS)
pentry = NULL;

Less code and less chances to miss 'else'.

PD> - in cache_inode_getattr.c : you need to keep the V_r. If not,
PD> the pentry will remain locked, which will result in deadlocks

Um, I'm adding the unlock, not removing it. And the reason I'm doing
it because it was locked previously and must be unlocked on the 'error
out' path.

In fact, what are you feelings about using goto's and error_out for
managing this kind of situations? It is similar to what Linux kernel
practices and it works there reasonably well.

PD> - in cache_inode_link.c : same issue
PD> - in cache_inode_rename.c : same issue too.

Once again, I'm unlocking on error, not removing the lock.

Compare that with cache_inode_rename.c where I've removed the lock
which was unlocked twice.

PD> The key here, is that cache_inode-* calls makes all of the lock
PD> management. If they take lock, when should release it when
PD> exiting. The statements that you removed were set for that
PD> purpose.

We're in violent agreement on that one.

PD> - in cache_inode_rdwr.c : why removing the test on
PD> if(seek_descriptor->whence != FSAL_SEEK_SET) ? Was it causing
PD> bugs ?

No, it was simply duplicated code - the same check is done earlier in
the same function.

And the 'missing' brace was causing vi to go crazy when trying to
match braces.

PD> In the current state, I can't integrate that commit (the change
PD> in cache_inode_get seems quite dangerous).

I agree on that one - do you want me to re-write this in the same
commit or add another one on top to fix the 'else' problem?

max