fgetattr() Not Being Called By fstat()

[Alex-Richman]

Hi,

If I call fstat() on a file within a fuse file system I only see an fgetattr() when the file is first created by the open call. If I call fstat() on an existing file, it uses getattr() instead.

I guess this isn't a big problem for most file systems, but getattr() is a very expensive operation on mine due to the path manipulation (whereas with fgetattr() the supplied fd can be used directly). Compounding this, some network file system daemons call fstat() after every write call (I assume to check that the file size is increasing), which kills the write speed.

I originally found this using a custom libfuse build based on 2.9.7, but I've also replicated it in the build shipped with Ubuntu 14 Trusty (I believe 2.9.2). I've also replicated it on the Ubuntu kernel (based on 3.19) and a custom gentoo kernel (based on 4.4 with some grsecurity mixed in).

Here's pseudo code for the replication:

fd = open(<path>, O_RDWR | O_CREAT):

for(iter = 0; iter < 3; iter++) {
    fstat(fd, <stbuf>);
    sleep(2);
}

close(fd);

Which results in these calls in fuse:

# Test run for first time, file created - We see one fgetattr then only getattrs #
getattr called! ('/test', 0x7f3ddbdcac20)
[f]getattr called! ('/test', 0x7f3ddb5c9be0, 0x7f3ddb5c9d10)
getattr called! ('/test', 0x7f3ddbdcabf0)
getattr called! ('/test', 0x7f3ddb5c9bf0)


# Test run again, file already exists - We only see getattrs #
getattr called! ('/test', 0x7f3ddbdcac20)
getattr called! ('/test', 0x7f3ddbdcabf0)
getattr called! ('/test', 0x7f3ddb5c9bf0)

I tested this using the fusexmp_fh.c source from the fuse 2.9.7 release, with some extra logging + options added (I've marked the custom bits with /* debug */ for clarity).

The full sources I used are attached below.

fusefgetattrbug.zip

Cheers,

• Alex.

[Alex-Richman]

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?

[Alex-Richman]

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.

[Nikratio]

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

[Alex-Richman]

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.

[jabolopes]

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?

[Nikratio]

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?

[jabolopes]

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

[jabolopes]

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.

[Nikratio]

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

[Nikratio]

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.

[Grian]

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.

[YorkTsai2012]

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;
}

[YorkTsai2012]

@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;
  }

}

[trapexit]

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?

[Nikratio]

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.

[trapexit]

Then this should be closed?

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

[Nikratio]

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.

[trapexit]

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.

[Nikratio]

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

[trapexit]

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.

[trapexit]

@jabolopes What ever happened with your patch?

[jabolopes]

It was not accepted by upstream. I can no longer find the thread. But I think we linked it here in Github somewhere, and I believe Nikolaus was CCed.

On Tue, Mar 12, 2019 at 3:13 PM trapexit [email protected] wrote:

@jabolopes https://github.com/jabolopes What ever happened with your patch?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libfuse/libfuse/issues/62#issuecomment-472017546, or mute the thread https://github.com/notifications/unsubscribe-auth/ABw9YEo9TyghLccv7fqMsA-P-LpH_GuXks5vV7YEgaJpZM4JOhC0 .

[jabolopes]

I found the thread: https://marc.info/?l=linux-fsdevel&m=147812724826542&w=2

On Tue, Mar 12, 2019 at 4:23 PM Jose Lopes [email protected] wrote:

It was not accepted by upstream. I can no longer find the thread. But I think we linked it here in Github somewhere, and I believe Nikolaus was CCed.

On Tue, Mar 12, 2019 at 3:13 PM trapexit [email protected] wrote:

@jabolopes https://github.com/jabolopes What ever happened with your patch?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libfuse/libfuse/issues/62#issuecomment-472017546, or mute the thread https://github.com/notifications/unsubscribe-auth/ABw9YEo9TyghLccv7fqMsA-P-LpH_GuXks5vV7YEgaJpZM4JOhC0 .

[trapexit]

Thanks.

I get the argument but 1) The file handle is already in the message. Someone obviously thought it'd be used. Data is already being transferred... it's just a matter of filling it in. 2) Setting it makes userland so much easier. Seems like a waste to not use.

On Tue, Mar 12, 2019 at 4:54 PM jabolopes [email protected] wrote:

I found the thread: https://marc.info/?l=linux-fsdevel&m=147812724826542&w=2

On Tue, Mar 12, 2019 at 4:23 PM Jose Lopes [email protected] wrote:

It was not accepted by upstream. I can no longer find the thread. But I think we linked it here in Github somewhere, and I believe Nikolaus was CCed.

On Tue, Mar 12, 2019 at 3:13 PM trapexit [email protected] wrote:

@jabolopes https://github.com/jabolopes What ever happened with your patch?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libfuse/libfuse/issues/62#issuecomment-472017546, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABw9YEo9TyghLccv7fqMsA-P-LpH_GuXks5vV7YEgaJpZM4JOhC0

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libfuse/libfuse/issues/62#issuecomment-472176952, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMELMr4VNlsDhZ7hM6KhrkYY6wL4tAxks5vWBP-gaJpZM4JOhC0 .