fgetattr() Not Being Called By fstat() about libfuse HOT 24 OPEN

from libfuse.

The relevant code path looks to be in lib/fuse.c:2642 fuse_lib_getattr, where if it's passed a null fi it'll call getattr instead of fgetattr.
This is called from lib/fuse_lowlevel.c:1092 do_getattr, which sets the fip based on the presence of the FUSE_GETATTR_FH flag in inargs.getattr_flags.
Via various layers, this is called by fs/fuse/dir.c:866 static int fuse_do_getattr in the kernel sources, which sets the FUSE_GETATTR_FH flag based on this check if (file && S_ISREG(inode->i_mode)) {.

Based on that, it may be a kernel issue not a libfuse issue?

from libfuse.

Added some debug printk()s to fs/fuse/dir.c and confirmed that the FUSE_GETATTR_FH flag isn't being set because the supplied file is NULL:

[  205.800058] fuse_do_getattr:            (nil), 1
[  205.800060]   Not setting FUSE_GETATTR_FH
[  207.800240] fuse_do_getattr:            (nil), 1
[  207.800243]   Not setting FUSE_GETATTR_FH

I only see 2 messages in the kernel logs here for the 3 fstat() calls - I assume this is because the open() call grabs the stats, which means the first fstat() hits the attr cache?

Here's the debug prints for reference:

/fs/fuse/dir.c:864 fuse_do_getattr:
        printk(KERN_INFO "fuse_do_getattr: %p, %d\n", file, S_ISREG(inode->i_mode));

        /* Directories have separate file-handle space */
        if (file && S_ISREG(inode->i_mode)) {
                printk(KERN_INFO "  Setting FUSE_GETATTR_FH\n");

                struct fuse_file *ff = file->private_data;

                inarg.getattr_flags |= FUSE_GETATTR_FH;
                inarg.fh = ff->fh;
        } else {
                printk(KERN_INFO "  Not setting FUSE_GETATTR_FH\n");
        }

I'm not super familiar with filesystems in the kernel, and can't find what is calling fuse_do_getattr with the NULL file. I'll ping this over to linux-fsdevel and see if anyone there has any ideas.

from libfuse.

Thanks for the report! Were you able to confirm that this is a kernel issue?

from libfuse.

Hey,

I strongly suspect that it is, but I'm not familiar enough with the kernel module to find out for sure.

Here's the thread on linux-fsdevel for reference: http://marc.info/?l=linux-fsdevel&m=146885856607447&w=2

Miklos may know, perhaps you could give him a friendly poke from me? :)

Cheers,

• Alex.

from libfuse.

Hi,

I was taking a look at the setattr on the kernel side. Looking at
http://lxr.free-electrons.com/source/fs/fuse/dir.c#L1709
it seems that the fs handlers that get the file handle are those for which
attr->ia_valid & ATTR_FILE
is true.

I believe this goes back to the syscalls. If we look at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/open.c
we see that ftruncate passes the file struct to do_truncate
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/open.c#n204
but for example fchmod does not pass the same struct to chmod_common
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/open.c#n555

The same applies to fchown, utimens, etc. If we pass the struct to these functions we can then
if (filp) {
newattrs.ia_file = filp;
newattrs.ia_valid |= ATTR_FILE;
}
and the fs handlers will get the file handle, just like the ftruncate handler does.

SInce I never did any kernel code, it would be great if we could ask the opinion of
someone who actually knows what they are doing.

What do you guys think?

from libfuse.

It sounds reasonable, but I'm not a kernel guy either. I think the best way forward is if you could put together a patch and send it to linux-kernel and linux-fsdevel.

Did you also look at the getattr path?

from libfuse.

I didn't look at getattr. I thought Alex or Miklos were looking at that.

from libfuse.

Hi,

Quick update. I wrote a simple patch to update fchmod like I suggested,
and recompiled my kernel with that patch.
I ran the same tests we talked about yesterday and this time it worked: if I call
fchmod I get the file handle, if I call chmod I don't, as expected.

I will try to submit the patch to the kernel ml this week.

from libfuse.

Great! Please also Cc fuse-devel when you submit the patch.

from libfuse.

Just for the record: at least in the low-level API, the current behavior is not actually a bug. The description of setattr() in fuse_lowlevel.h clearly says that fi is only defined if setattr() was called as a result of ftruncate. For getattr, fi is explicitly reserved for future use. Not saying that having it available wouldn't be an improvement though.

from libfuse.

I found when getattr returns { size = 0 } then kernel issues GETATTR request with FUSE_GETATTR_FH flag again, but response on this request is not used by kernel anyway.
It's kernel problem as I can see, because I am not using libfuse for reading requests from kernel.

from libfuse.

It is truly that a fstat can not always trigger a fgetattr of libfuse, even after v3.0.0 libfuse merges fgetattr to getattr by adding an extra argument struct fuse_file_info *fi.

I mean that a system-call fstat should trigger a fgetattr of libfuse before version 3.0.0
or a system-call fstat should trigger a getattr with fi not being NULL from version 3.0.0

Could you please take a look again and give a fix plan? @Nikratio @Alex-Richman

You libfuse code authors seem to know something tricky about the getattr.

The comment in the fuse.h v2.9.2

Currently this is only called after the create() method if that is implemented (see above).

 /**
   * Get attributes from an open file
   *
   * This method is called instead of the getattr() method if the
   * file information is available.
   *
   * Currently this is only called after the create() method if that
   * is implemented (see above).  Later it may be called for
   * invocations of fstat() too.
   *
   * Introduced in version 2.5
   */
  int (*fgetattr) (const char *, struct stat *, struct fuse_file_info *);

The comment in the fuse.h v3.0.0

fi will always be NULL if the file is not currenly open, but may also be NULL if the file is open.

  /** Get file attributes.
   *
   * Similar to stat().  The 'st_dev' and 'st_blksize' fields are
   * ignored. The 'st_ino' field is ignored except if the 'use_ino'
   * mount option is given.
   *
   * `fi` will always be NULL if the file is not currenly open, but
   * may also be NULL if the file is open.
   */
  int (*getattr) (const char *, struct stat *, struct fuse_file_info *fi);

Even I change my test to libfuse 3.0.0, a fstat not always trigger a getattr with fi not being NULL.

This bug, or as you thought, an enhancement gives some trouble when we use xtrabackup to access the fuse user space file system.
There is a fstat call in the xtrabackup, and it triggers a getattr, not a fgetattr, then we hit a crash.

  if (my_fstat(cursor->file.m_file, &cursor->statinfo, MYF(MY_WME))) {
    msg("[%02u] xtrabackup: error: cannot stat %s\n",
        thread_n, cursor->abs_path);

    xb_fil_cur_close(cursor);

    return(XB_FIL_CUR_ERROR);
  }

When you looply fstat the fd of an open file, the underling request is getattr, not fgetattr.
The fgetattr is only called, when you first create a file (not open an existed file).
The bellow is my testing code under the fuse mountpoint directory.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>

int main(int argc, char *argv[]) {

  if (argc < 2) {
    printf("test: fstat an open file\n");
    printf("usage: %s %s\n", argv[0], "file_name");
    exit(-1);
  }
  char *path = argv[1];
  int fd = open(path, O_CREAT|O_WRONLY|O_TRUNC, 0644);
  if (fd < 0) {
    printf("open err %d\n", errno);
    exit(errno);
  } else {
    printf("open ok fd %d\n", fd);
  }
  const char *data = " hello\n";
  struct stat st_buf;
  int round = 0;
  while (true) {
    ssize_t nbytes = write(fd, data, strlen(data) + 1);
    if (nbytes < 0) {
      printf("write data err %d\n", errno);
      exit(errno);
    } else {
      printf("write success nbytes %ld\n", nbytes);
    }
    memset((void*)&st_buf, 0, sizeof(struct stat));
    int ret = fstat(fd, &st_buf);
    if (ret < 0) {
      printf("fstat err: %d\n", errno);
      exit(errno);
    }
    printf("round: %d\n", round);
    printf("fstat nlink:%d\n", st_buf.st_nlink);
    printf("fstat size:%d\n", st_buf.st_size);
    sleep(1);
    round++;
  }
  close(fd);
  return 0;
}

from libfuse.

@Alex-Richman @jabolopes
I also encounter the fstat not calling fgetattr problem.
Do you have confirmed that the kernel fs/fuse hits a fuse_do_getattr bug?

In the kernel fs/fuse, fuse_getattr calls fuse_update_attributes passing file == NULL,
so fuse_update_attributes calls fuse_do_getattr with the param file == NULL, forever.

and there is not any chance to let inarg.getattr_flags |= FUSE_GETATTR_FH;

static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
         struct file *file)
{

  /* Directories have separate file-handle space */
  if (file && S_ISREG(inode->i_mode)) {
    struct fuse_file *ff = file->private_data;

    inarg.getattr_flags |= FUSE_GETATTR_FH;
    inarg.fh = ff->fh;
  }

}

from libfuse.

Any updates on this? @szmi is this a matter of it not being possible or difficult or is it just that no one has done it?

from libfuse.

This needs to be fixed in the kernel and the fix is rather invasive. Changing this has therefore been vetoed by the kernel maintainers. That discussion happened quite some time ago, you should find it somewhere in the linux-fsdevel archives.

from libfuse.

Then this should be closed?

That's really unfortunate as it seriously inhibits / breaks proper semantics wrt working on unlinked opened files.

from libfuse.

It remains a reasonable enhancement, even if there currently does not seem to be a way to implement it :-).

I don't think it breaks proper semantics though - you can either use the low-level API or the "hard_remove" functionality offered by the high-level API.

from libfuse.

Maybe I'm missing something but how would you manage a getattr on an unlinked file without fh? You can't correlate the request with a handle. hard_remove just forces an unlink rather than hiding the node and file. Any followup fstat will fail because you only have the node, not the handle.

from libfuse.

At the low-level, getattr receives the inode (which should still exist) rather than the pathname.

At the high-level, getattr after unlink should receive the pathname of the "hidden" file - which again still exists (I meant to not use hard_unlink).

In both cases, this should be enough information to return file attributes

from libfuse.

Sure, if you keep track of all file handles yourself and assume there is no functional difference between them for this purpose. That might not be true though. In the least permissions are different. Need to check vs not. stat vs fstat.

from libfuse.

@jabolopes What ever happened with your patch?

from libfuse.

from libfuse.

from libfuse.