Ceph » CephFS

I tried on latest Luminous RC + 4.12 kernel client. I got about 7000 opens/second in two nodes read-write case.
did you kernel client or fuse client? and which version?

I'm only running the fuse client. I see the problem both on Jewel (10.2.9 servers + fuse client) and on Luminous RC (12.1.2 servers + corresponding fuse client).

FUSE is the only caller of ->ll_lookup so a simpler fix might be to just change the mask field to 0 in the _lookup call in Client::ll_lookup. FUSE is the only caller of the ->ll_lookup operation, and it only uses st_ino, st_dev and st_rdev fields. We don't really need all of the caps there. e.g.:

diff --git a/src/client/Client.cc b/src/client/Client.cc
index b1aae051b0d1..11bc85ca1787 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -10092,7 +10092,8 @@ int Client::ll_lookup(Inode *parent, const char *name, struct stat *attr,
   string dname(name);
   InodeRef in;

-  r = _lookup(parent, dname, CEPH_STAT_CAP_INODE_ALL, &in, perms);
+  // FUSE only cares about st_dev, st_ino and st_rdev
+  r = _lookup(parent, dname, 0, &in, perms);
   if (r < 0) {
     attr->st_ino = 0;
     goto out;

what worry me is comment in fuse_lowlevel.h

struct fuse_entry_param {

...

        /** Inode attributes.
         *
         * Even if attr_timeout == 0, attr must be correct. For example,
         * for open(), FUSE uses attr.st_size from lookup() to determine
         * how many bytes to request. If this value is not correct,
         * incorrect data will be returned.
         */
        struct stat attr;

        /** Validity timeout (in seconds) for inode attributes. If
            attributes only change as a result of requests that come
            through the kernel, this should be set to a very large
            value. */
        double attr_timeout;

...

};

Ahh yeah, I remember seeing that in there a while back. I guess the danger is that we can end up instantiating an inode with almost completely bogus attributes.

Hmm...if that's the case then your original patch is also problematic, is it not? Could the inode could have gotten flushed out of the kernel core, while we still hold caps in the userland client? Kernel then issues another lookup later, and then we end up passing down an incomplete attr struct, and it instantiates the inode from that.

What we really ought to do is have fuse set some sort of flag in a new inode that the volatile attrs are bogus, and then use that flag to force a ->getattr on it before doing anything that requires up-to-date attrs.

That's a rather invasive change though if there's not already something like that in fuse that we're not aware of.

static ssize_t fuse_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
        struct inode *inode = iocb->ki_filp->f_mapping->host;
        struct fuse_conn *fc = get_fuse_conn(inode);

        /*
         * In auto invalidate mode, always update attributes on read.
         * Otherwise, only update if we attempt to read past EOF (to ensure
         * i_size is up to date).
         */
        if (fc->auto_inval_data ||
            (iocb->ki_pos + iov_iter_count(to) > i_size_read(inode))) {
                int err;
                err = fuse_update_attributes(inode, NULL, iocb->ki_filp, NULL);
                if (err)
                        return err;
        }

        return generic_file_read_iter(iocb, to);
}

The comment for st_size seems outdate. I agree that "zero mask" shoule be OK for ll_lookup

kernel of RHEL7 supports FUSE_AUTO_INVAL_DATA. But FUSE_CAP_DONT_MASK was added in libfuse 3.0. Currently no major linux distribution includes libfuse 3.0.0

Just checking if there is any viable patch that I could try for this issue in ceph-fuse. We are running into this problem in quite a few of our codes in a very painful way, so any help would be appreciated.