In message <[EMAIL PROTECTED]>,Rainer Toebbicke writes:
>I'd prefer a
>       osi_Assert(down_trylock(&ip->i_sem) == 0);

i wouldnt bother with the assert.  i already know this is the 
case.  but you want to use an assert be my guest.  i just want
to know if this patch fixes your problem.

>> +    if (down_trylock(&ip->i_sem))
>> +    need_unlock = 0;
>> +
>
>This doesn't look sane to me! If you manage to down the i_sem then 
>you're ok if the i_sem is really needed for filemap_fdatasync, however 
>if you don't then you don't really know whether it's because you 
>already did it yourself higher up or if "somebody else" (not 
>necessarily AFS code) did and is then dangerously close to messing 
>around with the structures.

if you read my previous mail, there are three paths into this code
section.  one already holds i_sem (the kernel takes it), and two
others dont.  i could take i_sem outside this routine for the others
(afs_linux_close/afs_linux_flush and BStore), but that's just changing
more files than i would like.  for a test its simple enough to see if we
should take the lock.

if you dont like the patch, dont try it.
_______________________________________________
OpenAFS-devel mailing list
[email protected]
https://lists.openafs.org/mailman/listinfo/openafs-devel

Reply via email to