From: Miklos Szeredi <miklos@szeredi.hu> To: Vivek Goyal <vgoyal@redhat.com> Cc: Amir Goldstein <amir73il@gmail.com>, Greg Kurz <groug@kaod.org>, virtualization@lists.linux-foundation.org, linux-fsdevel <linux-fsdevel@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, virtio-fs-list <virtio-fs@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com>, Robert Krawitz <rlk@redhat.com> Subject: Re: [PATCH v4 5/5] virtiofs: propagate sync() to file server Date: Mon, 30 Aug 2021 19:36:17 +0200 [thread overview] Message-ID: <CAJfpeguyBU3r+rUa9NvvNjaBwUno9O5PszAoriABStf5Fm3xfg@mail.gmail.com> (raw) In-Reply-To: <YS0O2MlR2G2LJH/0@redhat.com> On Mon, 30 Aug 2021 at 19:01, Vivek Goyal <vgoyal@redhat.com> wrote: > > static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi, > > @@ -1608,6 +1609,9 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa) > > struct fuse_args_pages *ap = &wpa->ia.ap; > > int i; > > > > + if (wpa->bucket && atomic_dec_and_test(&wpa->bucket->num_writepages)) > > Hi Miklos, > > Wondering why this wpa->bucket check is there. Isn't every wpa is associated > bucket. So when do we run into situation when wpa->bucket = NULL. In case fc->sync_fs is false. > > @@ -1871,6 +1875,19 @@ static struct fuse_writepage_args *fuse_writepage_args_alloc(void) > > > > } > > > > +static void fuse_writepage_add_to_bucket(struct fuse_conn *fc, > > + struct fuse_writepage_args *wpa) > > +{ > > + if (!fc->sync_fs) > > + return; > > + > > + rcu_read_lock(); > > + do { > > + wpa->bucket = rcu_dereference(fc->curr_bucket); > > + } while (unlikely(!atomic_inc_not_zero(&wpa->bucket->num_writepages))); > > So this loop is there because fuse_sync_fs() might be replacing > fc->curr_bucket. And we are fetching this pointer under rcu. So it is > possible that fuse_fs_sync() dropped its reference and that led to > ->num_writepages 0 and we don't want to use this bucket. > > What if fuse_sync_fs() dropped its reference but still there is another > wpa in progress and hence ->num_writepages is not zero. We still don't > want to use this bucket for new wpa, right? It's an unlikely race in which case the the write will go into the old bucket, and will be waited for, but that definitely should not be a problem. > > @@ -528,6 +542,31 @@ static int fuse_sync_fs(struct super_block *sb, int wait) > > if (!fc->sync_fs) > > return 0; > > > > + new_bucket = fuse_sync_bucket_alloc(); > > + spin_lock(&fc->lock); > > + bucket = fc->curr_bucket; > > + if (atomic_read(&bucket->num_writepages) != 0) { > > + /* One more for count completion of old bucket */ > > + atomic_inc(&new_bucket->num_writepages); > > + rcu_assign_pointer(fc->curr_bucket, new_bucket); > > + /* Drop initially added active count */ > > + atomic_dec(&bucket->num_writepages); > > + spin_unlock(&fc->lock); > > + > > + wait_event(bucket->waitq, atomic_read(&bucket->num_writepages) == 0); > > + /* > > + * Drop count on new bucket, possibly resulting in a completion > > + * if more than one syncfs is going on > > + */ > > + if (atomic_dec_and_test(&new_bucket->num_writepages)) > > + wake_up(&new_bucket->waitq); > > + kfree_rcu(bucket, rcu); > > + } else { > > + spin_unlock(&fc->lock); > > + /* Free unused */ > > + kfree(new_bucket); > When can we run into the situation when fc->curr_bucket is num_writepages > == 0. When install a bucket it has count 1. And only time it can go to > 0 is when we have dropped the initial reference. And initial reference > can be dropped only after removing bucket from fc->curr_bucket. > > IOW, we don't drop initial reference on a bucket if it is in > fc->curr_bucket. And that mean anything installed fc->curr_bucket should > not ever have a reference count of 0. What am I missing. You are correct. I fixed it by warning on zero count and checking for count != 1. I have other fixes as well, will send v2. Thanks, Miklos
WARNING: multiple messages have this Message-ID (diff)
From: Miklos Szeredi <miklos@szeredi.hu> To: Vivek Goyal <vgoyal@redhat.com> Cc: Amir Goldstein <amir73il@gmail.com>, linux-kernel <linux-kernel@vger.kernel.org>, virtio-fs-list <virtio-fs@redhat.com>, Max Reitz <mreitz@redhat.com>, linux-fsdevel <linux-fsdevel@vger.kernel.org>, virtualization@lists.linux-foundation.org, Robert Krawitz <rlk@redhat.com> Subject: Re: [Virtio-fs] [PATCH v4 5/5] virtiofs: propagate sync() to file server Date: Mon, 30 Aug 2021 19:36:17 +0200 [thread overview] Message-ID: <CAJfpeguyBU3r+rUa9NvvNjaBwUno9O5PszAoriABStf5Fm3xfg@mail.gmail.com> (raw) In-Reply-To: <YS0O2MlR2G2LJH/0@redhat.com> On Mon, 30 Aug 2021 at 19:01, Vivek Goyal <vgoyal@redhat.com> wrote: > > static struct fuse_writepage_args *fuse_find_writeback(struct fuse_inode *fi, > > @@ -1608,6 +1609,9 @@ static void fuse_writepage_free(struct fuse_writepage_args *wpa) > > struct fuse_args_pages *ap = &wpa->ia.ap; > > int i; > > > > + if (wpa->bucket && atomic_dec_and_test(&wpa->bucket->num_writepages)) > > Hi Miklos, > > Wondering why this wpa->bucket check is there. Isn't every wpa is associated > bucket. So when do we run into situation when wpa->bucket = NULL. In case fc->sync_fs is false. > > @@ -1871,6 +1875,19 @@ static struct fuse_writepage_args *fuse_writepage_args_alloc(void) > > > > } > > > > +static void fuse_writepage_add_to_bucket(struct fuse_conn *fc, > > + struct fuse_writepage_args *wpa) > > +{ > > + if (!fc->sync_fs) > > + return; > > + > > + rcu_read_lock(); > > + do { > > + wpa->bucket = rcu_dereference(fc->curr_bucket); > > + } while (unlikely(!atomic_inc_not_zero(&wpa->bucket->num_writepages))); > > So this loop is there because fuse_sync_fs() might be replacing > fc->curr_bucket. And we are fetching this pointer under rcu. So it is > possible that fuse_fs_sync() dropped its reference and that led to > ->num_writepages 0 and we don't want to use this bucket. > > What if fuse_sync_fs() dropped its reference but still there is another > wpa in progress and hence ->num_writepages is not zero. We still don't > want to use this bucket for new wpa, right? It's an unlikely race in which case the the write will go into the old bucket, and will be waited for, but that definitely should not be a problem. > > @@ -528,6 +542,31 @@ static int fuse_sync_fs(struct super_block *sb, int wait) > > if (!fc->sync_fs) > > return 0; > > > > + new_bucket = fuse_sync_bucket_alloc(); > > + spin_lock(&fc->lock); > > + bucket = fc->curr_bucket; > > + if (atomic_read(&bucket->num_writepages) != 0) { > > + /* One more for count completion of old bucket */ > > + atomic_inc(&new_bucket->num_writepages); > > + rcu_assign_pointer(fc->curr_bucket, new_bucket); > > + /* Drop initially added active count */ > > + atomic_dec(&bucket->num_writepages); > > + spin_unlock(&fc->lock); > > + > > + wait_event(bucket->waitq, atomic_read(&bucket->num_writepages) == 0); > > + /* > > + * Drop count on new bucket, possibly resulting in a completion > > + * if more than one syncfs is going on > > + */ > > + if (atomic_dec_and_test(&new_bucket->num_writepages)) > > + wake_up(&new_bucket->waitq); > > + kfree_rcu(bucket, rcu); > > + } else { > > + spin_unlock(&fc->lock); > > + /* Free unused */ > > + kfree(new_bucket); > When can we run into the situation when fc->curr_bucket is num_writepages > == 0. When install a bucket it has count 1. And only time it can go to > 0 is when we have dropped the initial reference. And initial reference > can be dropped only after removing bucket from fc->curr_bucket. > > IOW, we don't drop initial reference on a bucket if it is in > fc->curr_bucket. And that mean anything installed fc->curr_bucket should > not ever have a reference count of 0. What am I missing. You are correct. I fixed it by warning on zero count and checking for count != 1. I have other fixes as well, will send v2. Thanks, Miklos
next prev parent reply other threads:[~2021-08-30 17:36 UTC|newest] Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-20 15:46 [PATCH v4 0/5] virtiofs: propagate sync() to file server Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-20 15:46 ` [PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-20 19:45 ` Al Viro 2021-05-20 19:45 ` [Virtio-fs] " Al Viro 2021-05-20 19:45 ` Al Viro 2021-05-21 7:54 ` Miklos Szeredi 2021-05-21 7:54 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 8:15 ` Greg Kurz 2021-05-21 8:15 ` [Virtio-fs] " Greg Kurz 2021-05-21 8:15 ` Greg Kurz 2021-05-21 8:23 ` Miklos Szeredi 2021-05-21 8:23 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 8:08 ` Greg Kurz 2021-05-21 8:08 ` [Virtio-fs] " Greg Kurz 2021-05-21 8:08 ` Greg Kurz 2021-05-20 15:46 ` [PATCH v4 2/5] fuse: Call vfs_get_tree() for submounts Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-21 8:19 ` Miklos Szeredi 2021-05-21 8:19 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 8:28 ` Greg Kurz 2021-05-21 8:28 ` [Virtio-fs] " Greg Kurz 2021-05-21 8:28 ` Greg Kurz 2021-05-22 17:50 ` kernel test robot 2021-05-22 17:50 ` [Virtio-fs] " kernel test robot 2021-05-22 17:50 ` kernel test robot 2021-05-22 17:50 ` kernel test robot 2021-05-22 20:12 ` kernel test robot 2021-05-22 20:12 ` [Virtio-fs] " kernel test robot 2021-05-22 20:12 ` kernel test robot 2021-05-22 20:12 ` kernel test robot 2021-05-20 15:46 ` [PATCH v4 3/5] fuse: Make fuse_fill_super_submount() static Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-20 15:46 ` [PATCH v4 4/5] virtiofs: Skip submounts in sget_fc() Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-21 8:26 ` Miklos Szeredi 2021-05-21 8:26 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 8:39 ` Greg Kurz 2021-05-21 8:39 ` [Virtio-fs] " Greg Kurz 2021-05-21 8:39 ` Greg Kurz 2021-05-21 8:50 ` Miklos Szeredi 2021-05-21 8:50 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 10:06 ` Greg Kurz 2021-05-21 10:06 ` [Virtio-fs] " Greg Kurz 2021-05-21 10:06 ` Greg Kurz 2021-05-21 12:37 ` Miklos Szeredi 2021-05-21 12:37 ` [Virtio-fs] " Miklos Szeredi 2021-05-21 13:36 ` Greg Kurz 2021-05-21 13:36 ` [Virtio-fs] " Greg Kurz 2021-05-21 13:36 ` Greg Kurz 2021-05-20 15:46 ` [PATCH v4 5/5] virtiofs: propagate sync() to file server Greg Kurz 2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz 2021-05-20 15:46 ` Greg Kurz 2021-05-21 10:08 ` Greg Kurz 2021-05-21 10:08 ` [Virtio-fs] " Greg Kurz 2021-05-21 10:08 ` Greg Kurz 2021-05-21 12:51 ` Miklos Szeredi 2021-05-21 12:51 ` [Virtio-fs] " Miklos Szeredi 2021-08-15 14:14 ` Amir Goldstein 2021-08-15 14:14 ` [Virtio-fs] " Amir Goldstein 2021-08-16 15:29 ` Vivek Goyal 2021-08-16 15:29 ` [Virtio-fs] " Vivek Goyal 2021-08-16 15:29 ` Vivek Goyal 2021-08-16 18:57 ` Amir Goldstein 2021-08-16 18:57 ` [Virtio-fs] " Amir Goldstein 2021-08-16 19:11 ` Vivek Goyal 2021-08-16 19:11 ` [Virtio-fs] " Vivek Goyal 2021-08-16 19:11 ` Vivek Goyal 2021-08-16 19:46 ` Amir Goldstein 2021-08-16 19:46 ` [Virtio-fs] " Amir Goldstein 2021-08-28 15:21 ` Miklos Szeredi 2021-08-28 15:21 ` [Virtio-fs] " Miklos Szeredi 2021-08-30 17:01 ` Vivek Goyal 2021-08-30 17:01 ` [Virtio-fs] " Vivek Goyal 2021-08-30 17:01 ` Vivek Goyal 2021-08-30 17:36 ` Miklos Szeredi [this message] 2021-08-30 17:36 ` [Virtio-fs] " Miklos Szeredi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJfpeguyBU3r+rUa9NvvNjaBwUno9O5PszAoriABStf5Fm3xfg@mail.gmail.com \ --to=miklos@szeredi.hu \ --cc=amir73il@gmail.com \ --cc=groug@kaod.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mreitz@redhat.com \ --cc=rlk@redhat.com \ --cc=stefanha@redhat.com \ --cc=vgoyal@redhat.com \ --cc=virtio-fs@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.