Re: [External] Re: [PATCH] files: Use rcu lock to get the file structures for better performance

On Fri, May 22, 2020 at 12:47 AM Matthew Wilcox [off-list ref] wrote:

On Thu, May 21, 2020 at 08:38:35PM +0800, Muchun Song wrote:

quoted

+++ b/fs/proc/fd.c

@@ -34,19 +34,27 @@ static int seq_show(struct seq_file *m, void *v)
      if (files) {
              unsigned int fd = proc_fd(m->private);

-             spin_lock(&files->file_lock);
+             rcu_read_lock();
+again:
              file = fcheck_files(files, fd);
              if (file) {
-                     struct fdtable *fdt = files_fdtable(files);
+                     struct fdtable *fdt;
+
+                     if (!get_file_rcu(file)) {
+                             /*
+                              * we loop to catch the new file (or NULL
+                              * pointer).
+                              */
+                             goto again;
+                     }

+                     fdt = files_fdtable(files);

This is unusual, and may not be safe.

fcheck_files() loads files->fdt.  Then it loads file from fdt->fd[].
Now you're loading files->fdt again here, and it could have been changed
by another thread expanding the fd table.

You have to write a changelog which convinces me you've thought about
this race and that it's safe.  Because I don't think you even realise
it's a possibility at this point.

Thanks for your review, it is a problem. I can fix it.

quoted

@@ -160,14 +168,23 @@ static int proc_fd_link(struct dentry *dentry, struct path *path)
              unsigned int fd = proc_fd(d_inode(dentry));
              struct file *fd_file;

-             spin_lock(&files->file_lock);
+             rcu_read_lock();
+again:
              fd_file = fcheck_files(files, fd);
              if (fd_file) {
+                     if (!get_file_rcu(fd_file)) {
+                             /*
+                              * we loop to catch the new file
+                              * (or NULL pointer).
+                              */
+                             goto again;
+                     }
                      *path = fd_file->f_path;
                      path_get(&fd_file->f_path);
+                     fput(fd_file);
                      ret = 0;
              }
-             spin_unlock(&files->file_lock);
+             rcu_read_unlock();

Why is it an improvement to increment/decrement the refcount on the
struct file here, rather than take/release the spinlock?

lock-free vs spinlock.

Do you think spinlock would be better than the lock-free method?
Actually I prefer the rcu lock.

-- 
Yours,
Muchun