[RFC] fs: Prevent syncing frozen file system
diff mbox

Message ID 1436463945-12556-1-git-send-email-lczerner@redhat.com
State New
Headers show

Commit Message

Lukas Czerner July 9, 2015, 5:45 p.m. UTC
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(+)

Comments

Dave Chinner July 9, 2015, 11:40 p.m. UTC | #1
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.
Dave Chinner July 10, 2015, 12:49 a.m. UTC | #2
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.
Jan Kara July 10, 2015, 2:25 p.m. UTC | #3
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
Lukas Czerner July 10, 2015, 8:42 p.m. UTC | #4
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
Jan Kara July 13, 2015, 8:47 a.m. UTC | #5
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
> >

Patch
diff mbox

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;