Why does readdir() open files ?

Why does readdir() open files ?

[Ben RUBSON]

Hi,

I'm porting a FUSE FS to Cygwin, and I focus especially on readdir()  
performance.

What I see is that when readdir() occurs, each file of the directory is as  
expected (at least I think) stated (getattr), but also opened (open).
The first block of the file was even red, until I set the notexec mount  
option.

My question is, why is every file opened ?
This is quite a performance killer, particularly for a FUSE FS.

I thought this was to calculate the inode number, I then set the ihash  
mount option, but it did not help.

Thank you very much,

Best regards,

Ben

Re: Why does readdir() open files ?

[Ben RUBSON]

On 28 Mar 2018, Ben RUBSON wrote:

> Hi,
>
> I'm porting a FUSE FS to Cygwin, and I focus especially on readdir()  
> performance.
>
> What I see is that when readdir() occurs, each file of the directory is  
> as expected (at least I think) stated (getattr), but also opened (open).
> The first block of the file was even red, until I set the notexec mount  
> option.
>
> My question is, why is every file opened ?
> This is quite a performance killer, particularly for a FUSE FS.
>
> I thought this was to calculate the inode number, I then set the ihash  
> mount option, but it did not help.

I may be totally wrong, but perhaps the culprit is the following code path ?
https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2262
Perhaps we simply don't need to open the file if (!hasgood_inode ()) ?

That would be a nice performance improvement !

Thank you again,

Ben

Re: Why does readdir() open files ?

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Mar 28 18:35, Ben RUBSON wrote:
> On 28 Mar 2018, Ben RUBSON wrote:
> 
> > Hi,
> > 
> > I'm porting a FUSE FS to Cygwin, and I focus especially on readdir()
> > performance.
> > 
> > What I see is that when readdir() occurs, each file of the directory is
> > as expected (at least I think) stated (getattr), but also opened (open).
> > The first block of the file was even red, until I set the notexec mount
> > option.
> > 
> > My question is, why is every file opened ?
> > This is quite a performance killer, particularly for a FUSE FS.
> > 
> > I thought this was to calculate the inode number, I then set the ihash
> > mount option, but it did not help.
> 
> I may be totally wrong, but perhaps the culprit is the following code path ?
> https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2262
> Perhaps we simply don't need to open the file if (!hasgood_inode ()) ?
> 
> That would be a nice performance improvement !

You should make sure that the hasgood_inode() check works for your FUSE
FS.  Check at the end of fs_info::update():

  has_acls (flags () & FS_PERSISTENT_ACLS);
  /* Netapp inode numbers are fly-by-night. */
  hasgood_inode ((has_acls () && !is_netapp ()) || is_nfs ());

So your filesystem should return the FS_PERSISTENT_ACLS flags in a call
to NtQueryVolumeInformationFile(..., FileFsAttributeInformation)


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Why does readdir() open files ?

[Ben RUBSON]

On 28 Mar 2018 19:03, Corinna Vinschen wrote:

> On Mar 28 18:35, Ben RUBSON wrote:
>> On 28 Mar 2018, Ben RUBSON wrote:
>>
>>> Hi,
>>>
>>> I'm porting a FUSE FS to Cygwin, and I focus especially on readdir()
>>> performance.
>>>
>>> What I see is that when readdir() occurs, each file of the directory is
>>> as expected (at least I think) stated (getattr), but also opened (open).
>>> The first block of the file was even red, until I set the notexec mount
>>> option.
>>>
>>> My question is, why is every file opened ?
>>> This is quite a performance killer, particularly for a FUSE FS.
>>>
>>> I thought this was to calculate the inode number, I then set the ihash
>>> mount option, but it did not help.
>>
>> I may be totally wrong, but perhaps the culprit is the following code  
>> path ?
>> https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2262
>> Perhaps we simply don't need to open the file if (!hasgood_inode ()) ?
>>
>> That would be a nice performance improvement !
>
> You should make sure that the hasgood_inode() check works for your FUSE
> FS.  Check at the end of fs_info::update():
>
>   has_acls (flags () & FS_PERSISTENT_ACLS);
>   /* Netapp inode numbers are fly-by-night. */
>   hasgood_inode ((has_acls () && !is_netapp ()) || is_nfs ());
>
> So your filesystem should return the FS_PERSISTENT_ACLS flags in a call
> to NtQueryVolumeInformationFile(..., FileFsAttributeInformation)

Thank you for your answer Corinna.

You assume FS_PERSISTENT_ACLS must be set, otherwise I would not pass the  
following dir->__flags condition ?
if (de->d_ino == 0 && (dir->__flags & dirent_set_d_ino))
https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2225

So, as goal is to avoid opening files, I then should remove this flag from  
my FS ?

I found that hasgood_inode() is also defined in winsup/cygwin/path.h :
bool hasgood_inode () const {return !(path_flags & PATH_IHASH); }

And when I set the ihash mount option, it is correctly honoured : generated  
inode numbers differ from the "true" ones.
Can't we also rely on the ihash mount option to avoid opening files ?

Thank you again for your clarification & support !

Ben

Re: Why does readdir() open files ?

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On Mar 28 19:51, Ben RUBSON wrote:
> On 28 Mar 2018 19:03, Corinna Vinschen wrote:
> 
> > On Mar 28 18:35, Ben RUBSON wrote:
> > > On 28 Mar 2018, Ben RUBSON wrote:
> > > 
> > > > Hi,
> > > > 
> > > > I'm porting a FUSE FS to Cygwin, and I focus especially on readdir()
> > > > performance.
> > > > 
> > > > What I see is that when readdir() occurs, each file of the directory is
> > > > as expected (at least I think) stated (getattr), but also opened (open).
> > > > The first block of the file was even red, until I set the notexec mount
> > > > option.
> > > > 
> > > > My question is, why is every file opened ?
> > > > This is quite a performance killer, particularly for a FUSE FS.
> > > > 
> > > > I thought this was to calculate the inode number, I then set the ihash
> > > > mount option, but it did not help.
> > > 
> > > I may be totally wrong, but perhaps the culprit is the following
> > > code path ?
> > > https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2262
> > > Perhaps we simply don't need to open the file if (!hasgood_inode ()) ?
> > > 
> > > That would be a nice performance improvement !
> > 
> > You should make sure that the hasgood_inode() check works for your FUSE
> > FS.  Check at the end of fs_info::update():
> > 
> >   has_acls (flags () & FS_PERSISTENT_ACLS);
> >   /* Netapp inode numbers are fly-by-night. */
> >   hasgood_inode ((has_acls () && !is_netapp ()) || is_nfs ());
> > 
> > So your filesystem should return the FS_PERSISTENT_ACLS flags in a call
> > to NtQueryVolumeInformationFile(..., FileFsAttributeInformation)
> 
> Thank you for your answer Corinna.
> 
> You assume FS_PERSISTENT_ACLS must be set, otherwise I would not pass the
> following dir->__flags condition ?
> if (de->d_ino == 0 && (dir->__flags & dirent_set_d_ino))
> https://github.com/mirror/newlib-cygwin/blob/master/winsup/cygwin/fhandler_disk_file.cc#L2225
> 
> So, as goal is to avoid opening files, I then should remove this flag from
> my FS ?

What?  No!  This is not what I was talking about.

- Make sure the filesystem returns valid inodes and make sure
  to set FS_PERSISTENT_ACLS.

- Also make sure that NtQueryDirectoryFile(FileIdBothDirectoryInformation)
  works on your FS, and make sure to return a valid inode number in the
  structure.

This is the best option you have.  Everything else leads to workarounds
in Cygwin to make the results as POSIXy as possible.

> I found that hasgood_inode() is also defined in winsup/cygwin/path.h :
> bool hasgood_inode () const {return !(path_flags & PATH_IHASH); }
> 
> And when I set the ihash mount option,

Don't do that.  The ihash option is a workaround for inferior filesystems.
It's set by the path_conv routine if hasgood_inode fails, or the user
can set the ihash flag for a Cygwin mount point.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]