$ git diff
diff --git a/test/functional/libgfapi-python-tests.py b/test/functional/libgfapi-python-tests.py
index f6cc0fb..320ba4b 100644
--- a/test/functional/libgfapi-python-tests.py
+++ b/test/functional/libgfapi-python-tests.py
@@ -905,6 +905,7 @@ class DirOpsTest(unittest.TestCase):
         file_count = 0
         symlink_count = 0
         for index, (name, stat_info) in enumerate(dir_list_sorted):
+            print name, stat_info.st_mode
             self.assertTrue(isinstance(stat_info, Stat))
             if stat.S_ISREG(stat_info.st_mode):
                 self.assertEqual(stat_info.st_size, len(self.data))
@@ -914,6 +915,7 @@ class DirOpsTest(unittest.TestCase):
                 dir_count += 1
             elif stat.S_ISLNK(stat_info.st_mode):
                 symlink_count += 1
+        print self.vol.stat(os.path.join(self.dir_path, "testfile")).st_mode
         self.assertEqual(dir_count, 3)
         self.assertEqual(file_count, 1)
         self.assertEqual(symlink_count, 2)



$ nosetests -v --exe test/functional/libgfapi-python-tests.py:DirOpsTest.test_listdir_with_stat
test_listdir_with_stat (test.functional.libgfapi-python-tests.DirOpsTest) ... FAIL

======================================================================
FAIL: test_listdir_with_stat (test.functional.libgfapi-python-tests.DirOpsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ppai/src/libgfapi-python/test/functional/libgfapi-python-tests.py", line 920, in test_listdir_with_stat
    self.assertEqual(file_count, 1)
AssertionError: 0 != 1
-------------------- >> begin captured stdout << ---------------------
test_symlink_dir 41471
test_symlink_file 41471
testdir0 16895
testdir1 16895
testdir2 16895
testfile 0
33206

--------------------- >> end captured stdout << ----------------------

----------------------------------------------------------------------
Ran 1 test in 1.007s

FAILED (failures=1)



Seems like st_mode isn't populated in glfs_readdirplus_r() response for a regular file. A subsequent stat call gives the right result.

A simple reproducer:

#!/usr/bin/env python

import os
import stat
from gluster.gfapi import Volume

vol = Volume("localhost", "test")
vol.mount()

testdir = "some_dir1"

vol.mkdir(testdir, 0755)
file_path = os.path.join(testdir, "testfile")
with vol.fopen(file_path, 'w') as f:
    f.write("hello world")

# this internally calls glfs_readdirplus_r()
dir_list = vol.listdir_with_stat(testdir)
for index, (name, stat_info) in enumerate(dir_list):
    print name, stat_info.st_mode

This is confirmed to be reproduced with commit ba12c00f6d5b1e79796c1cb03f87035ebafd7f19 of release 5 branch

Created attachment 1493059 [details]
testcases (C and Python based)

Reconfirmed the issue using the tox test case and the reproducer python given, thanks.

As libgfapi-python has issues against master, wrote a C based gfapi reproducer for the same. While doing this discovered that readdir following a write causes the said problem.

Attaching various test scripts that help demonstrate the problem using both C and Python code for the same. (NOTE: Export LD_LIBRARY_PATH to /usr/local/lib if building and installing from sources and testing, also python version will work only in the release branches afaik)

Using the C based reproducer in comment #4 bisected the code to determine that the failure is from the following patch:

commit c9bde3021202f1d5c5a2d19ac05a510fc1f788ac
https://review.gluster.org/c/glusterfs/+/20639

The commit message for the above patch does call out a race of the nature observed as follows,

<commit message>
Some fops don't have valid iatts in their responses. For eg., write
  response whose data is still cached in write-behind will have zeroed
  out stat. In this case keep only ia_type and ia_gfid and reset rest
  of the iatt members to zero.
  - fuse-bridge in this case just sends "entry" information back to
    kernel and attr is not sent.
  - gfapi sets entry->inode to NULL and zeroes out the entire stat
* There is one tiny race between the entry creation and a readdirp on
  its parent dir, which could cause the inode-ctx setting and inode
  ctx reading to happen on two different inode objects. To prevent
  this, when entry->inode doesn't eqaul to linked_inode,
  - fuse-bridge is made to send only "entry" information without
    attributes
  - gfapi sets entry->inode to NULL and zeroes out the entire stat.

The bug in this case seems to stem from one of the above (or maybe a pattern that is new) but looks simple enough that it needs to work as intended.

Requesting @krutika or @du to take a look and see how best to address the problem.

One additional comment, when using O_SYNC flag with write, the issue is not observed, possibly due to bypassing write behind and receiving right iatt from the brick.

(In reply to Prashanth Pai from comment #2)
> A simple reproducer:
> 
> #!/usr/bin/env python
> 
> import os
> import stat
> from gluster.gfapi import Volume
> 
> vol = Volume("localhost", "test")
> vol.mount()
> 
> testdir = "some_dir1"
> 
> vol.mkdir(testdir, 0755)
> file_path = os.path.join(testdir, "testfile")
> with vol.fopen(file_path, 'w') as f:
>     f.write("hello world")
> 
> # this internally calls glfs_readdirplus_r()
> dir_list = vol.listdir_with_stat(testdir)
> for index, (name, stat_info) in enumerate(dir_list):
>     print name, stat_info.st_mode

I forgot mention. The incorrect output here is that st_mode is set to 0.

Is this issue seen only on gfapi? What about fuse mounts?

Following is the likely hypothesis:

* readdir-ahead on seeing a write invalidates "attribute" (rest of iatt other than gfid and ia_type) information keeping "entry" (gfid, ia_type) information, by zeroing out the iatt corresponding to "attribute" part of iatt.
* Fuse-bridge if it sees zeroed out attribute information, sets attribute_valid=0 to indicate kernel to consume only entry information and asking it to do explicit stats for attribute information.

However, on gfapi this option of selectively sending entry information may not be available and hence the entire iatt is to be invalidated and no inode information returned (just like readdir).

@Poornima/@niels/@soumya/@Shyam,

How does gfapi handle dentries with zeroed out stats? What information does it send back to application?

REVIEW: https://review.gluster.org/21396 (api: zero out entire stat if attribute information is not valid) posted (#1) for review on master by Raghavendra G

(In reply to Raghavendra G from comment #8)
> Is this issue seen only on gfapi? What about fuse mounts?

I don't know. I don't know which specific FUSE mount operation will internally trigger a glfs_readdirplus_r().

The libgfapi reproducer is simple and consistent - both the Python and C versions.

(In reply to Prashanth Pai from comment #11)
> (In reply to Raghavendra G from comment #8)
> > Is this issue seen only on gfapi? What about fuse mounts?
> 
> I don't know. I don't know which specific FUSE mount operation will
> internally trigger a glfs_readdirplus_r().
> 
> The libgfapi reproducer is simple and consistent - both the Python and C
> versions.

No fuse function invokes glfs_readdirplus_r. So, I'll assume the issue is observed only for gfapi.

(In reply to Raghavendra G from comment #9)
> Following is the likely hypothesis:
> 
> * readdir-ahead on seeing a write invalidates "attribute" (rest of iatt
> other than gfid and ia_type) information keeping "entry" (gfid, ia_type)
> information, by zeroing out the iatt corresponding to "attribute" part of
> iatt.
> * Fuse-bridge if it sees zeroed out attribute information, sets
> attribute_valid=0 to indicate kernel to consume only entry information and
> asking it to do explicit stats for attribute information.
> 
> However, on gfapi this option of selectively sending entry information may
> not be available and hence the entire iatt is to be invalidated and no inode
> information returned (just like readdir).
> 
> @Poornima/@niels/@soumya/@Shyam,
> 
> How does gfapi handle dentries with zeroed out stats? What information does
> it send back to application?


FWIU from the code, it sends back that NULL stat to the application as is which is incorrect as the applications are not aware and shall not do any special handling for such cases. 

I think those cases are handled by setting entry->inode to NULL so that gfapi does lookup on that dirent before filling up the stat. 

int glfd_entry_refresh(struct glfs_fd *glfd, int plus) {
  xlator_t *subvol = NULL;
.....
...
  if (ret >= 0) {
    if (plus) {
      list_for_each_entry(entry, &entries.list, list) {
        if (!entry->inode && !IA_ISDIR(entry->d_stat.ia_type)) {
          /* entry->inode for directories will be
           * always set to null to force a lookup
           * on the dentry. Also we will have
           * proper stat if directory present on
           * hashed subvolume.
           */
          gf_fill_iatt_for_dirent(entry, fd->inode, subvol);
        }
      }
....
...

Here 'gf_fill_iatt_for_dirent' does lookup if entry->inode is NULL. So readdir-ahead xlator should set entry->inode to NULL whenever it needs to invalidate iatt, so that gfapi can perform lookup. 

However I fail to understand/remembr why the above check skips directory entries. Wouldn't it result in skipping directories with entry->inode=NULL in the readdir(p) response.

Okay after a bit of thorough reading, I see that we do not skip dir entries (with entry->inode NULL) in the response but just do not perform lookup/link their inodes so that any further operation on that dirent, shall force lookup.

So I see two approaches to fix this particular issue -
* From the commit-msg,
"translators like readdir-ahead selectively retain entry information of iatt (gfid and type) when rest of the iatt is invalidated (for write invalidating ia_size, (m)(c)times etc)."

Since readdir-ahead needs to invalidate attr for only files but not directories, it can set entry->inode to NULL and gfapi shall perform lookup to link inode and fetch stat before sending the attr to application

* or if this case needs to be handled for directories as well -->

we need extra check in the above routine " glfd_entry_refresh()" to validate stat and perform lookup if NULL. 

Thoughts?

There are (unused?) IATT_*_VALID() macros that check if an iatt has been filled with the attribute that is requested (through iatt->ia_attributes_mask?). Maybe we need inspection of all the attributes that are expected to be returned by glfs_readdirp_r() and (re)validate in case some of those attributes are not set.

Based on comment #14 and comment #15, here is what I think we need to answer, and also some possible thoughts:

1) What is the contract between application calling glfs_readdirplus(_r) and gfapi?

IMO,
- return entry with known stat information about the same

2) What is the contract between various xlators and returned iatt information and inode information?
- iatt validity seems to be ctime check, which is not the way forward, we need to adapt to IATT_*_VALID flags in the future
- inode NULL or not? Currently as the write has occured there would be an inode linked and returning a NULL seems logically incorrect, iatt invalidation seems to be the better option IMO
- Du?

3) What is the contact between protocol layer (in this case gfapi) and xlator?
- I would assume it is the same as (2) and by protocol I mean gfapi/FUSE(xlator) etc.

From an iatt and its validity fields, we need to adapt and address the IATT_*_VALID flags across the stack, and take this as the contract forward, but ca

(In reply to Shyamsundar from comment #16)
> Based on comment #14 and comment #15, here is what I think we need to
> answer, and also some possible thoughts:
> 
> 1) What is the contract between application calling glfs_readdirplus(_r) and
> gfapi?
> 
> IMO,
> - return entry with known stat information about the same
> 
> 2) What is the contract between various xlators and returned iatt
> information and inode information?
> - iatt validity seems to be ctime check, which is not the way forward, we
> need to adapt to IATT_*_VALID flags in the future
> - inode NULL or not? Currently as the write has occured there would be an
> inode linked and returning a NULL seems logically incorrect, iatt
> invalidation seems to be the better option IMO
> - Du?
> 
> 3) What is the contact between protocol layer (in this case gfapi) and
> xlator?
> - I would assume it is the same as (2) and by protocol I mean
> gfapi/FUSE(xlator) etc.
> 
> From an iatt and its validity fields, we need to adapt and address the
> IATT_*_VALID flags across the stack, and take this as the contract forward,
> but ca

+1

Du need inputs here to understand what to do next. Provided fix will not address the problem, please see comments above. Thanks.

(In reply to Soumya Koduri from comment #14)
> Okay after a bit of thorough reading, I see that we do not skip dir entries
> (with entry->inode NULL) in the response but just do not perform lookup/link
> their inodes so that any further operation on that dirent, shall force
> lookup.
> 
> So I see two approaches to fix this particular issue -
> * From the commit-msg,
> "translators like readdir-ahead selectively retain entry information of iatt
> (gfid and type) when rest of the iatt is invalidated (for write invalidating
> ia_size, (m)(c)times etc)."
> 
> Since readdir-ahead needs to invalidate attr for only files but not
> directories, it can set entry->inode to NULL and gfapi shall perform lookup
> to link inode and fetch stat before sending the attr to application

Can't gfapi do the same if attr is invalid (even though entry information is valid)? I am suggesting this because fuse-bridge separates entry and attribute information and setting entry->inode to NULL (by readdir-ahead) means fuse-bridge cannot use entry information even though its valid.

> 
> * or if this case needs to be handled for directories as well -->
> 
> we need extra check in the above routine " glfd_entry_refresh()" to validate
> stat and perform lookup if NULL. 

What is the value of stats returned to application if dentry points to directory?

> 
> Thoughts?

(In reply to Raghavendra G from comment #19)
> (In reply to Soumya Koduri from comment #14)
> > Okay after a bit of thorough reading, I see that we do not skip dir entries
> > (with entry->inode NULL) in the response but just do not perform lookup/link
> > their inodes so that any further operation on that dirent, shall force
> > lookup.
> > 
> > So I see two approaches to fix this particular issue -
> > * From the commit-msg,
> > "translators like readdir-ahead selectively retain entry information of iatt
> > (gfid and type) when rest of the iatt is invalidated (for write invalidating
> > ia_size, (m)(c)times etc)."
> > 
> > Since readdir-ahead needs to invalidate attr for only files but not
> > directories, it can set entry->inode to NULL and gfapi shall perform lookup
> > to link inode and fetch stat before sending the attr to application
> 
> Can't gfapi do the same if attr is invalid (even though entry information is
> valid)? I am suggesting this because fuse-bridge separates entry and
> attribute information and setting entry->inode to NULL (by readdir-ahead)
> means fuse-bridge cannot use entry information even though its valid.

yes.. it can ..that's the below option which I mentioned and even Shyam & Niels suggest that we can use IATT_*_VALID macros to have those checks.
> 
> > 
> > * or if this case needs to be handled for directories as well -->
> > 
> > we need extra check in the above routine " glfd_entry_refresh()" to validate
> > stat and perform lookup if NULL. 
> 
> What is the value of stats returned to application if dentry points to
> directory?

We send the dentry info along with stats as is ..just that inode shall not be created & linked if not present already so that it shall be looked upon on any next operation (I think it was requirement from dht/tiering to set need_lookup flag for directory entries...dont remember the exact reason)

> 
> > 
> > Thoughts?

(In reply to Shyamsundar from comment #16)
> Based on comment #14 and comment #15, here is what I think we need to
> answer, and also some possible thoughts:
> 
> 1) What is the contract between application calling glfs_readdirplus(_r) and
> gfapi?
> 
> IMO,
> - return entry with known stat information about the same
> 
> 2) What is the contract between various xlators and returned iatt
> information and inode information?
> - iatt validity seems to be ctime check, which is not the way forward, we
> need to adapt to IATT_*_VALID flags in the future

+1

> - inode NULL or not? Currently as the write has occured there would be an
> inode linked and returning a NULL seems logically incorrect, iatt
> invalidation seems to be the better option IMO
> - Du?

This can be done. However, not sure about the scope of change as the present code implicitly assumes entry->inode being NULL as a proxy for stat not present.

> 
> 3) What is the contact between protocol layer (in this case gfapi) and
> xlator?
> - I would assume it is the same as (2) and by protocol I mean
> gfapi/FUSE(xlator) etc.
> 
> From an iatt and its validity fields, we need to adapt and address the
> IATT_*_VALID flags across the stack, and take this as the contract forward,
> but ca

(In reply to Raghavendra G from comment #21)
> (In reply to Shyamsundar from comment #16)
> > Based on comment #14 and comment #15, here is what I think we need to
> > answer, and also some possible thoughts:
> > 
> > 1) What is the contract between application calling glfs_readdirplus(_r) and
> > gfapi?
> > 
> > IMO,
> > - return entry with known stat information about the same
> > 
> > 2) What is the contract between various xlators and returned iatt
> > information and inode information?
> > - iatt validity seems to be ctime check, which is not the way forward, we
> > need to adapt to IATT_*_VALID flags in the future
> 
> +1

This could/would be the future, for now I think we need to work with ctime and not flags, as when readdir-ahead is not present in the stack and we fix readdir-ahead to return the flags, this assumption would break again and fail.

> 
> > - inode NULL or not? Currently as the write has occured there would be an
> > inode linked and returning a NULL seems logically incorrect, iatt
> > invalidation seems to be the better option IMO
> > - Du?
> 
> This can be done. However, not sure about the scope of change as the present
> code implicitly assumes entry->inode being NULL as a proxy for stat not
> present.

I suggest iatt->ia_ctime being 0 as the trigger, which I understand is done in other places.

If we agree to the above, changes would be in gfapi only, to resolve/stat the inode again for iatt information.

Also, we need to conclude fast, just so we can release with the fix. Unless there is disagreement that this is a blocker.

> 
> > 
> > 3) What is the contact between protocol layer (in this case gfapi) and
> > xlator?
> > - I would assume it is the same as (2) and by protocol I mean
> > gfapi/FUSE(xlator) etc.
> > 
> > From an iatt and its validity fields, we need to adapt and address the
> > IATT_*_VALID flags across the stack, and take this as the contract forward,
> > but ca

(In reply to Shyamsundar from comment #22)
> > 
> > > - inode NULL or not? Currently as the write has occured there would be an
> > > inode linked and returning a NULL seems logically incorrect, iatt
> > > invalidation seems to be the better option IMO
> > > - Du?
> > 
> > This can be done. However, not sure about the scope of change as the present
> > code implicitly assumes entry->inode being NULL as a proxy for stat not
> > present.
> 
> I suggest iatt->ia_ctime being 0 as the trigger, which I understand is done
> in other places.
> 
> If we agree to the above, changes would be in gfapi only, to resolve/stat
> the inode again for iatt information.
> 
> Also, we need to conclude fast, just so we can release with the fix. Unless
> there is disagreement that this is a blocker.

https://review.gluster.org/21396 already does that. But ppai reported its not fixing the issue. I would need help from gfapi maintainers to review the patch and take it to completion. Also, please feel free to modify the patch and merge.

(In reply to Raghavendra G from comment #23)
> (In reply to Shyamsundar from comment #22)
> > > 
> > > > - inode NULL or not? Currently as the write has occured there would be an
> > > > inode linked and returning a NULL seems logically incorrect, iatt
> > > > invalidation seems to be the better option IMO
> > > > - Du?
> > > 
> > > This can be done. However, not sure about the scope of change as the present
> > > code implicitly assumes entry->inode being NULL as a proxy for stat not
> > > present.
> > 
> > I suggest iatt->ia_ctime being 0 as the trigger, which I understand is done
> > in other places.
> > 
> > If we agree to the above, changes would be in gfapi only, to resolve/stat
> > the inode again for iatt information.
> > 
> > Also, we need to conclude fast, just so we can release with the fix. Unless
> > there is disagreement that this is a blocker.
> 
> https://review.gluster.org/21396 already does that. But ppai reported its
> not fixing the issue. I would need help from gfapi maintainers to review the
> patch and take it to completion. Also, please feel free to modify the patch
> and merge.

It'll be good to add Shyam's C based reproducer as a test in the patch so that edits/updates to the patch can surely tell if the fix works or not.

(In reply to Prashanth Pai from comment #24)
> It'll be good to add Shyam's C based reproducer as a test in the patch so
> that edits/updates to the patch can surely tell if the fix works or not.

Added.

Latest version of https://review.gluster.org/#/c/glusterfs/+/21396/ fixes the issue

COMMIT: https://review.gluster.org/21396 committed in master by "Shyamsundar Ranganathan" <srangana> with a commit message- api: fill out attribute information if not valid

translators like readdir-ahead selectively retain entry information of
iatt (gfid and type) when rest of the iatt is invalidated (for write
invalidating ia_size, (m)(c)times etc). Fuse-bridge uses this
information and sends only entry information in readdirplus
response. However such option doesn't exist in gfapi. This patch
modifies gfapi to populate the stat by forcing an extra lookup.

Thanks to Shyamsundar Ranganathan <srangana> and Prashanth
Pai <ppai> for tests.

Change-Id: Ieb5f8fc76359c327627b7d8420aaf20810e53000
Fixes: bz#1630804
Signed-off-by: Raghavendra Gowdappa <rgowdapp>
Signed-off-by: Soumya Koduri <skoduri>

REVIEW: https://review.gluster.org/21443 (api: fill out attribute information if not valid) posted (#1) for review on release-5 by Shyamsundar Ranganathan

I confirm that with the fix (https://review.gluster.org/21443), all libgfapi-python functional tests currently pass against release 5 branch: https://paste.fedoraproject.org/paste/F7chXsoXM69Ev1wIGAzxJw

COMMIT: https://review.gluster.org/21443 committed in release-5 by "Shyamsundar Ranganathan" <srangana> with a commit message- api: fill out attribute information if not valid

translators like readdir-ahead selectively retain entry information of
iatt (gfid and type) when rest of the iatt is invalidated (for write
invalidating ia_size, (m)(c)times etc). Fuse-bridge uses this
information and sends only entry information in readdirplus
response. However such option doesn't exist in gfapi. This patch
modifies gfapi to populate the stat by forcing an extra lookup.

Thanks to Shyamsundar Ranganathan <srangana> and Prashanth
Pai <ppai> for tests.

Change-Id: Ieb5f8fc76359c327627b7d8420aaf20810e53000
Fixes: bz#1630804
Signed-off-by: Raghavendra Gowdappa <rgowdapp>
Signed-off-by: Soumya Koduri <skoduri>
(cherry picked from commit 6257276d9de3f15643f159b2ec627a67c84fc23d)

This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-5.0, please open a new bug report.

glusterfs-5.0 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] https://lists.gluster.org/pipermail/announce/2018-October/000115.html
[2] https://www.gluster.org/pipermail/gluster-users/

This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-6.0, please open a new bug report.

glusterfs-6.0 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] https://lists.gluster.org/pipermail/announce/2019-March/000120.html
[2] https://www.gluster.org/pipermail/gluster-users/