| Message ID | 1436463945-12556-1-git-send-email-lczerner@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 09, 2015 at 07:45:45PM +0200, Lukas Czerner wrote: > Currently we can end up in a deadlock because of broken > sb_start_write -> s_umount ordering. > > The race goes like this: > > - write the file > - unlink the file - final_iput will not be calles as file is opened > - freeze the file system > - Now simultaneously close the file and call sync (or syncfs on that > particular file system). Sync will get to wait_sb_inodes() where it will > grab the referece to the inode (__iget()) and later to call iput(). This problem goes away with the sync scalability patchset that josef has been trying to get merged: git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling That patchset removes the full sb inodes list walk in wait_sb_inodes() and replaces it with a walk of inodes cleaned during the sync, which will be an empty list in the case of sync running on an empty filesystem. This commit does the work: https://git.kernel.org/cgit/linux/kernel/git/josef/btrfs-next.git/commit/?h=superblock-scaling&id=9bea30d5f4521db674203f365b1e0970588b2650 <As a separate note, can we *please* get that patchset merged given that there are now several outstanding issues that it fixes in one go?> > If we manage to close the file and drop the reference in between those > calls sync will attempt to do a iput_final() because the inode is now > unlinked and we're holding the last reference to it. This will > however block on a frozen file system (ext4_delete_inode for > example). > > Note that I've not been able to reproduce the issue, I've only seen this > happen once. However with some instrumentation (like msleep() in the > wait_sb_inodes() it can be achieved. > > Fix this by properly doing sb_start_write/sb_end_write to prevent us > from fsfreeze. > > Note that with this patch syncfs will block on the frozen file system > which is probably ok, but sync will block if any file system happens to > be frozen - not sure if that's a problem, but it's certainly different > from what we've been used to. sync should not block on frozen fileystems. By definition, a frozen filesystem is a clean filesystem, and so sync should really just be skipping over them. > +++ b/fs/super.c > @@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) > sb->s_count++; > spin_unlock(&sb_lock); > > + /* > + * Whatever we're going to do to the file system we have to > + * make sure that we'll not end up blocking on frozen file > + * system. > + */ > + sb_start_write(sb); > down_read(&sb->s_umount); > if (sb->s_root && (sb->s_flags & MS_BORN)) > f(sb, arg); > up_read(&sb->s_umount); > + sb_end_write(sb); > > spin_lock(&sb_lock); > if (p) That deadlocks sysrq-j (emergency thaw)... Cheers, Dave.
On Fri, Jul 10, 2015 at 09:40:12AM +1000, Dave Chinner wrote: > On Thu, Jul 09, 2015 at 07:45:45PM +0200, Lukas Czerner wrote: > > Currently we can end up in a deadlock because of broken > > sb_start_write -> s_umount ordering. > > > > The race goes like this: > > > > - write the file > > - unlink the file - final_iput will not be calles as file is opened > > - freeze the file system > > - Now simultaneously close the file and call sync (or syncfs on that > > particular file system). Sync will get to wait_sb_inodes() where it will > > grab the referece to the inode (__iget()) and later to call iput(). > > This problem goes away with the sync scalability patchset that josef > has been trying to get merged: > > git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling > > That patchset removes the full sb inodes list walk in > wait_sb_inodes() and replaces it with a walk of inodes cleaned > during the sync, which will be an empty list in the case of sync > running on an empty filesystem. This commit does the work: s/empty/frozen -Dave.
On Fri 10-07-15 09:40:12, Dave Chinner wrote: > On Thu, Jul 09, 2015 at 07:45:45PM +0200, Lukas Czerner wrote: > > Currently we can end up in a deadlock because of broken > > sb_start_write -> s_umount ordering. > > > > The race goes like this: > > > > - write the file > > - unlink the file - final_iput will not be calles as file is opened > > - freeze the file system > > - Now simultaneously close the file and call sync (or syncfs on that > > particular file system). Sync will get to wait_sb_inodes() where it will > > grab the referece to the inode (__iget()) and later to call iput(). > > This problem goes away with the sync scalability patchset that josef > has been trying to get merged: > > git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling > > That patchset removes the full sb inodes list walk in > wait_sb_inodes() and replaces it with a walk of inodes cleaned > during the sync, which will be an empty list in the case of sync > running on an empty filesystem. This commit does the work: > > https://git.kernel.org/cgit/linux/kernel/git/josef/btrfs-next.git/commit/?h=superblock-scaling&id=9bea30d5f4521db674203f365b1e0970588b2650 > > <As a separate note, can we *please* get that patchset merged given > that there are now several outstanding issues that it fixes in one > go?> Not sure where that got stuck - oh, maybe on Tejun's memcg writeback series which was clashing with it. Josef? > > If we manage to close the file and drop the reference in between those > > calls sync will attempt to do a iput_final() because the inode is now > > unlinked and we're holding the last reference to it. This will > > however block on a frozen file system (ext4_delete_inode for > > example). > > > > Note that I've not been able to reproduce the issue, I've only seen this > > happen once. However with some instrumentation (like msleep() in the > > wait_sb_inodes() it can be achieved. > > > > Fix this by properly doing sb_start_write/sb_end_write to prevent us > > from fsfreeze. > > > > Note that with this patch syncfs will block on the frozen file system > > which is probably ok, but sync will block if any file system happens to > > be frozen - not sure if that's a problem, but it's certainly different > > from what we've been used to. > > sync should not block on frozen fileystems. By definition, a frozen > filesystem is a clean filesystem, and so sync should really just be > skipping over them. Just for record I agree with Dave. Sync on frozen fs should just return. And freeze protection in iterate_supers() looks just wrong. Honza > > +++ b/fs/super.c > > @@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) > > sb->s_count++; > > spin_unlock(&sb_lock); > > > > + /* > > + * Whatever we're going to do to the file system we have to > > + * make sure that we'll not end up blocking on frozen file > > + * system. > > + */ > > + sb_start_write(sb); > > down_read(&sb->s_umount); > > if (sb->s_root && (sb->s_flags & MS_BORN)) > > f(sb, arg); > > up_read(&sb->s_umount); > > + sb_end_write(sb); > > > > spin_lock(&sb_lock); > > if (p) > > That deadlocks sysrq-j (emergency thaw)... > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 10 Jul 2015, Jan Kara wrote: > Date: Fri, 10 Jul 2015 16:25:25 +0200 > From: Jan Kara <jack@suse.cz> > To: Dave Chinner <david@fromorbit.com> > Cc: Lukas Czerner <lczerner@redhat.com>, viro@ZenIV.linux.org.uk, > bfields@fieldses.org, linux-kernel@vger.kernel.org, > linux-fsdevel@vger.kernel.org > Subject: Re: [RFC][PATCH] fs: Prevent syncing frozen file system > > On Fri 10-07-15 09:40:12, Dave Chinner wrote: > > On Thu, Jul 09, 2015 at 07:45:45PM +0200, Lukas Czerner wrote: > > > Currently we can end up in a deadlock because of broken > > > sb_start_write -> s_umount ordering. > > > > > > The race goes like this: > > > > > > - write the file > > > - unlink the file - final_iput will not be calles as file is opened > > > - freeze the file system > > > - Now simultaneously close the file and call sync (or syncfs on that > > > particular file system). Sync will get to wait_sb_inodes() where it will > > > grab the referece to the inode (__iget()) and later to call iput(). > > > > This problem goes away with the sync scalability patchset that josef > > has been trying to get merged: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling > > > > That patchset removes the full sb inodes list walk in > > wait_sb_inodes() and replaces it with a walk of inodes cleaned > > during the sync, which will be an empty list in the case of sync > > running on an empty filesystem. This commit does the work: > > > > https://git.kernel.org/cgit/linux/kernel/git/josef/btrfs-next.git/commit/?h=superblock-scaling&id=9bea30d5f4521db674203f365b1e0970588b2650 > > > > <As a separate note, can we *please* get that patchset merged given > > that there are now several outstanding issues that it fixes in one > > go?> > > Not sure where that got stuck - oh, maybe on Tejun's memcg writeback series > which was clashing with it. Josef? > > > > If we manage to close the file and drop the reference in between those > > > calls sync will attempt to do a iput_final() because the inode is now > > > unlinked and we're holding the last reference to it. This will > > > however block on a frozen file system (ext4_delete_inode for > > > example). > > > > > > Note that I've not been able to reproduce the issue, I've only seen this > > > happen once. However with some instrumentation (like msleep() in the > > > wait_sb_inodes() it can be achieved. > > > > > > Fix this by properly doing sb_start_write/sb_end_write to prevent us > > > from fsfreeze. > > > > > > Note that with this patch syncfs will block on the frozen file system > > > which is probably ok, but sync will block if any file system happens to > > > be frozen - not sure if that's a problem, but it's certainly different > > > from what we've been used to. > > > > sync should not block on frozen fileystems. By definition, a frozen > > filesystem is a clean filesystem, and so sync should really just be > > skipping over them. > > Just for record I agree with Dave. Sync on frozen fs should just return. > And freeze protection in iterate_supers() looks just wrong. Sure, that's why it's rfc. Anyway with the change Dave mentioned the deadlock should not be possible anymore. However anywhere where we take s_umount before sb_start_write we could deadlock, so it might be worth adding a warning into sb_start_write() maybe ? -Lukas > > Honza > > > > +++ b/fs/super.c > > > @@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) > > > sb->s_count++; > > > spin_unlock(&sb_lock); > > > > > > + /* > > > + * Whatever we're going to do to the file system we have to > > > + * make sure that we'll not end up blocking on frozen file > > > + * system. > > > + */ > > > + sb_start_write(sb); > > > down_read(&sb->s_umount); > > > if (sb->s_root && (sb->s_flags & MS_BORN)) > > > f(sb, arg); > > > up_read(&sb->s_umount); > > > + sb_end_write(sb); > > > > > > spin_lock(&sb_lock); > > > if (p) > > > > That deadlocks sysrq-j (emergency thaw)... > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 10-07-15 22:42:40, Lukáš Czerner wrote: > On Fri, 10 Jul 2015, Jan Kara wrote: > > > Date: Fri, 10 Jul 2015 16:25:25 +0200 > > From: Jan Kara <jack@suse.cz> > > To: Dave Chinner <david@fromorbit.com> > > Cc: Lukas Czerner <lczerner@redhat.com>, viro@ZenIV.linux.org.uk, > > bfields@fieldses.org, linux-kernel@vger.kernel.org, > > linux-fsdevel@vger.kernel.org > > Subject: Re: [RFC][PATCH] fs: Prevent syncing frozen file system > > > > On Fri 10-07-15 09:40:12, Dave Chinner wrote: > > > On Thu, Jul 09, 2015 at 07:45:45PM +0200, Lukas Czerner wrote: > > > > Currently we can end up in a deadlock because of broken > > > > sb_start_write -> s_umount ordering. > > > > > > > > The race goes like this: > > > > > > > > - write the file > > > > - unlink the file - final_iput will not be calles as file is opened > > > > - freeze the file system > > > > - Now simultaneously close the file and call sync (or syncfs on that > > > > particular file system). Sync will get to wait_sb_inodes() where it will > > > > grab the referece to the inode (__iget()) and later to call iput(). > > > > > > This problem goes away with the sync scalability patchset that josef > > > has been trying to get merged: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git superblock-scaling > > > > > > That patchset removes the full sb inodes list walk in > > > wait_sb_inodes() and replaces it with a walk of inodes cleaned > > > during the sync, which will be an empty list in the case of sync > > > running on an empty filesystem. This commit does the work: > > > > > > https://git.kernel.org/cgit/linux/kernel/git/josef/btrfs-next.git/commit/?h=superblock-scaling&id=9bea30d5f4521db674203f365b1e0970588b2650 > > > > > > <As a separate note, can we *please* get that patchset merged given > > > that there are now several outstanding issues that it fixes in one > > > go?> > > > > Not sure where that got stuck - oh, maybe on Tejun's memcg writeback series > > which was clashing with it. Josef? > > > > > > If we manage to close the file and drop the reference in between those > > > > calls sync will attempt to do a iput_final() because the inode is now > > > > unlinked and we're holding the last reference to it. This will > > > > however block on a frozen file system (ext4_delete_inode for > > > > example). > > > > > > > > Note that I've not been able to reproduce the issue, I've only seen this > > > > happen once. However with some instrumentation (like msleep() in the > > > > wait_sb_inodes() it can be achieved. > > > > > > > > Fix this by properly doing sb_start_write/sb_end_write to prevent us > > > > from fsfreeze. > > > > > > > > Note that with this patch syncfs will block on the frozen file system > > > > which is probably ok, but sync will block if any file system happens to > > > > be frozen - not sure if that's a problem, but it's certainly different > > > > from what we've been used to. > > > > > > sync should not block on frozen fileystems. By definition, a frozen > > > filesystem is a clean filesystem, and so sync should really just be > > > skipping over them. > > > > Just for record I agree with Dave. Sync on frozen fs should just return. > > And freeze protection in iterate_supers() looks just wrong. > > Sure, that's why it's rfc. Anyway with the change Dave mentioned the > deadlock should not be possible anymore. However anywhere where we > take s_umount before sb_start_write we could deadlock, so it might > be worth adding a warning into sb_start_write() maybe ? Lockdep should warn you about that since fs freezing is properly hooked into it... Honza > > > > +++ b/fs/super.c > > > > @@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) > > > > sb->s_count++; > > > > spin_unlock(&sb_lock); > > > > > > > > + /* > > > > + * Whatever we're going to do to the file system we have to > > > > + * make sure that we'll not end up blocking on frozen file > > > > + * system. > > > > + */ > > > > + sb_start_write(sb); > > > > down_read(&sb->s_umount); > > > > if (sb->s_root && (sb->s_flags & MS_BORN)) > > > > f(sb, arg); > > > > up_read(&sb->s_umount); > > > > + sb_end_write(sb); > > > > > > > > spin_lock(&sb_lock); > > > > if (p) > > > > > > That deadlocks sysrq-j (emergency thaw)... > > > > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@fromorbit.com > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
diff --git a/fs/super.c b/fs/super.c index b613723..d337c91 100644 --- a/fs/super.c +++ b/fs/super.c @@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) sb->s_count++; spin_unlock(&sb_lock); + /* + * Whatever we're going to do to the file system we have to + * make sure that we'll not end up blocking on frozen file + * system. + */ + sb_start_write(sb); down_read(&sb->s_umount); if (sb->s_root && (sb->s_flags & MS_BORN)) f(sb, arg); up_read(&sb->s_umount); + sb_end_write(sb); spin_lock(&sb_lock); if (p) diff --git a/fs/sync.c b/fs/sync.c index fbc98ee..074247f 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -156,9 +156,17 @@ SYSCALL_DEFINE1(syncfs, int, fd) return -EBADF; sb = f.file->f_path.dentry->d_sb; + /* + * If the file system is frozen we can't proceed because we + * could potentially block on frozen file system. This would + * lead to a deadlock, because we'll be holding s_umount which + * has to be taken in order to thaw the file system as well + */ + sb_start_write(sb); down_read(&sb->s_umount); ret = sync_filesystem(sb); up_read(&sb->s_umount); + sb_end_write(sb); fdput(f); return ret;
Currently we can end up in a deadlock because of broken sb_start_write -> s_umount ordering. The race goes like this: - write the file - unlink the file - final_iput will not be calles as file is opened - freeze the file system - Now simultaneously close the file and call sync (or syncfs on that particular file system). Sync will get to wait_sb_inodes() where it will grab the referece to the inode (__iget()) and later to call iput(). If we manage to close the file and drop the reference in between those calls sync will attempt to do a iput_final() because the inode is now unlinked and we're holding the last reference to it. This will however block on a frozen file system (ext4_delete_inode for example). Note that I've not been able to reproduce the issue, I've only seen this happen once. However with some instrumentation (like msleep() in the wait_sb_inodes() it can be achieved. Fix this by properly doing sb_start_write/sb_end_write to prevent us from fsfreeze. Note that with this patch syncfs will block on the frozen file system which is probably ok, but sync will block if any file system happens to be frozen - not sure if that's a problem, but it's certainly different from what we've been used to. Signed-off-by: Lukas Czerner <lczerner@redhat.com> --- fs/super.c | 7 +++++++ fs/sync.c | 8 ++++++++ 2 files changed, 15 insertions(+)