[Pvfs2-developers] kernel readdir question

[Sam Lang]

On Nov 9, 2006, at 11:36 AM, Murali Vilayannur wrote:

> Sam,
>
>>  From what I can tell the directory_version gets set to the mtime
>> after the first upcall.  If future upcalls changed the mtime, we used
>> to start over, and we didn't want to call filldir again on . and ..
>
> Correct..
>>
>> But we recently changed this code to not start over if the directory
>> version changed from one readdir call to the next (it was causing
>> duplicate entries, see:  http://www.beowulf-underground.org/ 
>> pipermail/
>> pvfs2-developers/2006-October/002739.html), instead we just update
>> the directory version stored in the inode.  There's also the diff of
>> the Murali's changes:
>>
>> http://www.pvfs.org/cgi-bin/pvfs2/viewcvs/viewcvs.cgi/pvfs2/src/
>> kernel/linux-2.6/dir.c.diff?r1=1.45&r2=1.46
>
> Oh yeah. I forgot about that. :)
>
>> The directory_version gets set back to 0 once we hit the end of a
>> directory, so I could imagine that killing an ls while its listing a
>> directory with many entries would cause the directory version to be
>> set to the mtime of the last readdir call before the abort.  This
>> would give the behavior of the . and .. not showing up in the next
>> listing, but as long as that listing was allowed to complete, the
>> directory_version would be set back to 0 again.
>
> Well, if an ls was killed the directory is closed. Next time you start
> it up it will reopen
> the directory and start from offset 0. So i don't think that should be
> a problem.
> We keep the version for every open directory, so there should not be
> any state across
> process lifetimes.
>
>> Given that we removed the code that restarts if the version changes
>> on us midway through a listing, can we assume that the position will
>> be set to 0 _only_ on the first readdir call?  This would allow us to
>> remove the directory_version == 0 check.  In fact we should be able
>> to remove the PVFS_READDIR_END check that sets the directory_version
>> back to 0 as well...
>
> position is basically copied over from file->f_pos which is updated on
> every succesful
> filldir callback. So it should be set to 0 when we start out.
>
Agreed, but the directory_version field will be non-zero, since we  
didn't let the readdirs complete the listing.  This prevents the .  
and .. from getting added on the next ls.

> To me it now looks like we can get rid of the directory_version field
> itself from the pvfs2_inode structure and remove all associated checks
> as well.
> Please don't remove the if (pos == PVFS_READDIR_END) check though, we
> need to retain that to return 0, but we can discard setting the
> directory_version back to 0.

I think we're in agreement then.

>
> If you guys are busy, I can cook up a patch later today or tomorrow..
> let me know if you guys plan to take a stab at it.

I think I can look at it if that's alright.  I also want to see if I  
can reproduce the missing . and .. first.

-sam

> thanks,
> Murali
>
>> -sam
>>
>> > thanks,
>> > Murali
>> >
>> > On 11/8/06, Phil Carns <pcarns at wastedcycles.org> wrote:
>> >> It's been a while since I've seen this bug first hand, but I am
>> >> just now
>> >> getting around to looking at it.
>> >>
>> >> Every once in a while we have seen cases where "ls -al" in a pvfs2
>> >> directory fails to show the "." and ".." entries.  I _think_  
>> this has
>> >> mainly occurred after restarting pvfs2-client and/or pvfs2-server,
>> >> but I
>> >> am not certain.  I can't seem to reproduce it.
>> >>
>> >> At any rate, looking at the code in dir.c, it seems like filling
>> >> in the
>> >> "." and ".." entries should be pretty much automatic.  However,
>> >> there is
>> >> an if statement wrapped around the filldir() calls that looks like
>> >> this:
>> >>
>> >>          if (pvfs2_inode->directory_version == 0)
>> >>          {
>> >>
>> >> Anyone know what the purpose if this check is?  It seems to me
>> >> like "."
>> >> and ".." should be entries for position 0 and 1 regardless of the
>> >> directory version, but I may be missing something.
>> >>
>> >> -Phil
>> >> _______________________________________________
>> >> Pvfs2-developers mailing list
>> >> Pvfs2-developers at beowulf-underground.org
>> >> http://www.beowulf-underground.org/mailman/listinfo/pvfs2- 
>> developers
>> >>
>> > _______________________________________________
>> > Pvfs2-developers mailing list
>> > Pvfs2-developers at beowulf-underground.org
>> > http://www.beowulf-underground.org/mailman/listinfo/pvfs2- 
>> developers
>> >
>>
>>
>