Bug 1630804 - libgfapi-python: test_listdir_with_stat and test_scandir failure on release 5 branch
Summary: libgfapi-python: test_listdir_with_stat and test_scandir failure on release 5...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: GlusterFS
Classification: Community
Component: libgfapi
Version: 5
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Raghavendra G
QA Contact: bugs@gluster.org
URL:
Whiteboard:
Depends On:
Blocks: glusterfs-5.0
TreeView+ depends on / blocked
 
Reported: 2018-09-19 10:23 UTC by Prashanth Pai
Modified: 2019-03-25 16:30 UTC (History)
8 users (show)

Fixed In Version: glusterfs-6.0
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-10-23 15:19:19 UTC
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:


Attachments (Terms of Use)
testcases (C and Python based) (10.00 KB, application/x-tar)
2018-10-11 21:24 UTC, Shyamsundar
no flags Details

Description Prashanth Pai 2018-09-19 10:23:23 UTC
glusterfs version:

$ glusterfs --version
glusterfs 5.0rc0

Commit on top of release 5 branch: f4594a3c88cb0a76e260010d57632dc1c718fd5f


Reproducing steps:

1. Install release 5 branch from source
2. git clone https://github.com/gluster/libgfapi-python; cd libgfapi-python
3. yum install pip; pip install tox
4. tox -e functest27

[ppai@gd2-1 libgfapi-python]$ tox -e functest
GLOB sdist-make: /home/ppai/src/libgfapi-python/setup.py
functest inst-nodeps: /home/ppai/src/libgfapi-python/.tox/dist/gfapi-1.2.zip
functest installed: coverage==4.5.1,flake8==2.2.4,funcsigs==1.0.2,gfapi==1.2,hacking==0.10.3,mccabe==0.2.1,mock==2.0.0,nose==1.3.7,nosehtmloutput==0.0.5,nosexcover==1.0.11,pbr==4.2.0,pep8==1.5.7,pyflakes==0.8.1,six==1.11.0
functest runtests: PYTHONHASHSEED='3915143630'
functest runtests: commands[0] | nosetests -s -v test/functional
test_bin_open_and_read (test.functional.libgfapi-python-tests.BinFileOpsTest) ... ok
test_copy_tree (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_isdir (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_listdir (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_listdir_with_stat (test.functional.libgfapi-python-tests.DirOpsTest) ... FAIL
test_makedirs (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_rmtree (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_scandir (test.functional.libgfapi-python-tests.DirOpsTest) ... FAIL
test_statvfs (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_walk_default (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_walk_error (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_walk_no_topdown_and_followlinks (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_walk_no_topdown_no_followlinks (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_walk_topdown_and_followinks (test.functional.libgfapi-python-tests.DirOpsTest) ... ok
test_access (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_chmod (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copy (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copy2 (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copyfile_samefile (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copyfileobj (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copymode (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_copystat (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_create_file_already_exists (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_discard (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_double_close (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_exists (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_exists_false (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fallocate (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fgetxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_flistxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fopen (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fopen_err (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fopen_in_thread (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fremovexattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_fsetxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_ftruncate (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_getcwd_and_chdir (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_getsize (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_getxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_glfd_decorators_IO_on_invalid_glfd (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_isdir_false (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_isfile (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_islink_false (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_link (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_listxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_lstat (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_mknod (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_open_and_read (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_open_err (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_open_file_not_exist (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_readinto (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_readlink (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_removexattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_rename (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_setxattr (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_stat (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_symlink (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_unlink (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_utime (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_write_file_dup_lseek_read (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_zerofill (test.functional.libgfapi-python-tests.FileOpsTest) ... ok
test_get_volume_id (test.functional.libgfapi-python-tests.TestVolumeInit) ... ok
test_mount_err (test.functional.libgfapi-python-tests.TestVolumeInit) ... ok
test_mount_umount_default (test.functional.libgfapi-python-tests.TestVolumeInit) ... ok
test_set_logging (test.functional.libgfapi-python-tests.TestVolumeInit) ... ok
test_unix_socket_mount (test.functional.libgfapi-python-tests.TestVolumeInit) ... SKIP: Unix socket file /var/run/glusterd.socket not accessible
Reads test.conf config file which contains configurable options ... ok

======================================================================
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 918, in test_listdir_with_stat
    self.assertEqual(file_count, 1)
AssertionError: 0 != 1

======================================================================
FAIL: test_scandir (test.functional.libgfapi-python-tests.DirOpsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ppai/src/libgfapi-python/test/functional/libgfapi-python-tests.py", line 948, in test_scandir
    self.assertEqual(file_count, 1)
AssertionError: 0 != 1

Name                          Stmts   Miss Branch BrPart  Cover
---------------------------------------------------------------
gluster/__init__.py               2      0      0      0   100%
gluster/gfapi/__init__.py         3      0      0      0   100%
gluster/gfapi/api.py             91     14     12      3    78%
gluster/gfapi/exceptions.py       6      0      0      0   100%
gluster/gfapi/gfapi.py          772    150    306     62    78%
gluster/gfapi/utils.py           24      2      4      1    89%
---------------------------------------------------------------
TOTAL                           898    166    322     66    79%
----------------------------------------------------------------------
Ran 67 tests in 10.136s

FAILED (SKIP=1, failures=2)

Comment 1 Prashanth Pai 2018-09-19 11:06:51 UTC
$ 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.

Comment 2 Prashanth Pai 2018-09-20 09:52:39 UTC
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

Comment 3 Prashanth Pai 2018-10-11 05:41:20 UTC
This is confirmed to be reproduced with commit ba12c00f6d5b1e79796c1cb03f87035ebafd7f19 of release 5 branch

Comment 4 Shyamsundar 2018-10-11 21:24:29 UTC
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)

Comment 5 Shyamsundar 2018-10-11 21:29:18 UTC
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.

Comment 6 Shyamsundar 2018-10-11 21:45:37 UTC
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.

Comment 7 Prashanth Pai 2018-10-12 03:53:00 UTC
(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.

Comment 8 Raghavendra G 2018-10-12 04:36:00 UTC
Is this issue seen only on gfapi? What about fuse mounts?

Comment 9 Raghavendra G 2018-10-12 04:46:29 UTC
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?

Comment 10 Worker Ant 2018-10-12 05:06:24 UTC
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

Comment 11 Prashanth Pai 2018-10-12 06:11:16 UTC
(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.

Comment 12 Raghavendra G 2018-10-12 06:13:33 UTC
(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.

Comment 13 Soumya Koduri 2018-10-12 07:29:06 UTC
(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.

Comment 14 Soumya Koduri 2018-10-12 07:41:47 UTC
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?

Comment 15 Niels de Vos 2018-10-12 08:21:17 UTC
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.

Comment 16 Shyamsundar 2018-10-12 15:37:54 UTC
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

Comment 17 Soumya Koduri 2018-10-12 15:59:42 UTC
(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

Comment 18 Shyamsundar 2018-10-15 14:44:54 UTC
Du need inputs here to understand what to do next. Provided fix will not address the problem, please see comments above. Thanks.

Comment 19 Raghavendra G 2018-10-16 01:50:38 UTC
(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?

Comment 20 Soumya Koduri 2018-10-16 05:46:49 UTC
(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?

Comment 21 Raghavendra G 2018-10-16 06:06:51 UTC
(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

Comment 22 Shyamsundar 2018-10-16 14:21:30 UTC
(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

Comment 23 Raghavendra G 2018-10-17 03:35:32 UTC
(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.

Comment 24 Prashanth Pai 2018-10-17 03:48:09 UTC
(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.

Comment 25 Raghavendra G 2018-10-17 04:17:10 UTC
(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.

Comment 26 Raghavendra G 2018-10-17 04:36:13 UTC
Latest version of https://review.gluster.org/#/c/glusterfs/+/21396/ fixes the issue

Comment 27 Worker Ant 2018-10-17 21:41:08 UTC
COMMIT: https://review.gluster.org/21396 committed in master by "Shyamsundar Ranganathan" <srangana@redhat.com> 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@redhat.com> and Prashanth
Pai <ppai@redhat.com> for tests.

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

Comment 28 Worker Ant 2018-10-17 23:37:32 UTC
REVIEW: https://review.gluster.org/21443 (api: fill out attribute information if not valid) posted (#1) for review on release-5 by Shyamsundar Ranganathan

Comment 29 Prashanth Pai 2018-10-18 05:07:13 UTC
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

Comment 30 Worker Ant 2018-10-18 12:48:23 UTC
COMMIT: https://review.gluster.org/21443 committed in release-5 by "Shyamsundar Ranganathan" <srangana@redhat.com> 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@redhat.com> and Prashanth
Pai <ppai@redhat.com> for tests.

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

Comment 31 Shyamsundar 2018-10-23 15:19:19 UTC
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/

Comment 32 Shyamsundar 2019-03-25 16:30:43 UTC
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/


Note You need to log in before you can comment on or make changes to this bug.