[PATCH 0/2] Fix deadlock with suspend & quotas

[PATCH 0/2] Fix deadlock with suspend & quotas

[Jan Kara]

  Hello Al,

  This is my attempt to fix deadlock in quota code when filesystem is
suspended and someone tries to modify quota e.g. via Q_SETQUOTA command. I
added a VFS function to get a superblock reference and return only after
the filesystem is thawed. Is that what you had in mind? Can you merge
these patches? Thanks.

								Honza

[PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

In quota code we need to find a superblock corresponding to a device and wait
for superblock to be unfrozen. However this waiting has to happen without
s_umount semaphore because that is required for superblock to thaw. So provide
a function in VFS for this to keep dances with s_umount where they belong.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |    1 +
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 6015c02..e320239 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
 
 EXPORT_SYMBOL(iterate_supers_type);
 
-/**
- *	get_super - get the superblock of a device
- *	@bdev: device to get the superblock for
- *	
- *	Scans the superblock list and finds the superblock of the file system
- *	mounted on the device given. %NULL is returned if no match is found.
- */
-
-struct super_block *get_super(struct block_device *bdev)
+static struct super_block *__get_super(struct block_device *bdev, bool thawed)
 {
 	struct super_block *sb;
 
@@ -618,9 +610,13 @@ rescan:
 			spin_unlock(&sb_lock);
 			down_read(&sb->s_umount);
 			/* still alive? */
-			if (sb->s_root && (sb->s_flags & MS_BORN))
-				return sb;
-			up_read(&sb->s_umount);
+			if (sb->s_root && (sb->s_flags & MS_BORN)) {
+				if (sb->s_frozen == SB_UNFROZEN)
+					return sb;
+				up_read(&sb->s_umount);
+				vfs_check_frozen(sb, SB_FREEZE_WRITE);
+			} else
+				up_read(&sb->s_umount);
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
 			__put_super(sb);
@@ -631,9 +627,36 @@ rescan:
 	return NULL;
 }
 
+/**
+ *	get_super - get the superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device given. %NULL is returned if no match is found.
+ */
+
+struct super_block *get_super(struct block_device *bdev)
+{
+	return __get_super(bdev, false);
+}
 EXPORT_SYMBOL(get_super);
 
 /**
+ *	get_super_thawed - get thawed superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device given once the superblock is thawed. %NULL is
+ *	returned if no match is found.
+ */
+
+struct super_block *get_super_thawed(struct block_device *bdev)
+{
+	return __get_super(bdev, true);
+}
+EXPORT_SYMBOL(get_super_thawed);
+
+/**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
  *
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 386da09..69cd5bb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2496,6 +2496,7 @@ extern void get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
+extern struct super_block *get_super_thawed(struct block_device *);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
-- 
1.7.1

[PATCH 2/2] quota: Fix deadlock with suspend and quotas

[Jan Kara]

This script causes a kernel deadlock:
set -e
DEVICE=/dev/vg1/linear
lvchange -ay $DEVICE
mkfs.ext3 $DEVICE
mount -t ext3 -o usrquota,grpquota $DEVICE /mnt/test
quotacheck -gu /mnt/test
umount /mnt/test
mount -t ext3 -o usrquota,grpquota $DEVICE /mnt/test
quotaon /mnt/test
dmsetup suspend $DEVICE
setquota -u root 1 2 3 4 /mnt/test &
sleep 1
dmsetup resume $DEVICE

setquota acquired semaphore s_umount for read and then tried to perform a
transaction (and waits because the device is suspended).  dmsetup resume tries
to acquire s_umount for write before resuming the device (and waits for
setquota).

Fix the deadlock by grabbing a thawed superblock for quota commands which need
it.

Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/quota.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 7898cd6..fc2c438 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -292,11 +292,26 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 	}
 }
 
+/* Return 1 if 'cmd' will block on frozen filesystem */
+static int quotactl_cmd_write(int cmd)
+{
+	switch (cmd) {
+	case Q_GETFMT:
+	case Q_GETINFO:
+	case Q_SYNC:
+	case Q_XGETQSTAT:
+	case Q_XGETQUOTA:
+	case Q_XQUOTASYNC:
+		return 0;
+	}
+	return 1;
+}
+
 /*
  * look up a superblock on which quota ops will be performed
  * - use the name of a block device to find the superblock thereon
  */
-static struct super_block *quotactl_block(const char __user *special)
+static struct super_block *quotactl_block(const char __user *special, int cmd)
 {
 #ifdef CONFIG_BLOCK
 	struct block_device *bdev;
@@ -309,7 +324,10 @@ static struct super_block *quotactl_block(const char __user *special)
 	putname(tmp);
 	if (IS_ERR(bdev))
 		return ERR_CAST(bdev);
-	sb = get_super(bdev);
+	if (quotactl_cmd_write(cmd))
+		sb = get_super_thawed(bdev);
+	else
+		sb = get_super(bdev);
 	bdput(bdev);
 	if (!sb)
 		return ERR_PTR(-ENODEV);
@@ -361,7 +379,7 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special,
 			pathp = &path;
 	}
 
-	sb = quotactl_block(special);
+	sb = quotactl_block(special, cmds);
 	if (IS_ERR(sb)) {
 		ret = PTR_ERR(sb);
 		goto out;
-- 
1.7.1

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Dave Chinner]

On Tue, Feb 07, 2012 at 11:37:46PM +0100, Jan Kara wrote:
> In quota code we need to find a superblock corresponding to a device and wait
> for superblock to be unfrozen. However this waiting has to happen without
> s_umount semaphore because that is required for superblock to thaw. So provide
> a function in VFS for this to keep dances with s_umount where they belong.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
>  include/linux/fs.h |    1 +
>  2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 6015c02..e320239 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
>  
>  EXPORT_SYMBOL(iterate_supers_type);
>  
> -/**
> - *	get_super - get the superblock of a device
> - *	@bdev: device to get the superblock for
> - *	
> - *	Scans the superblock list and finds the superblock of the file system
> - *	mounted on the device given. %NULL is returned if no match is found.
> - */
> -
> -struct super_block *get_super(struct block_device *bdev)
> +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
>  {
>  	struct super_block *sb;
>  
> @@ -618,9 +610,13 @@ rescan:
>  			spin_unlock(&sb_lock);
>  			down_read(&sb->s_umount);
>  			/* still alive? */
> -			if (sb->s_root && (sb->s_flags & MS_BORN))
> -				return sb;
> -			up_read(&sb->s_umount);
> +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> +				if (sb->s_frozen == SB_UNFROZEN)
> +					return sb;
> +				up_read(&sb->s_umount);
> +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> +			} else
> +				up_read(&sb->s_umount);
>  			/* nope, got unmounted */
>  			spin_lock(&sb_lock);
>  			__put_super(sb);
> @@ -631,9 +627,36 @@ rescan:
>  	return NULL;
>  }

The thawed parameter appears to be unused and the waiting for frozen
filesystems appears to happen for all callers. Is this intentional?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

On Thu 09-02-12 10:20:49, Dave Chinner wrote:
> On Tue, Feb 07, 2012 at 11:37:46PM +0100, Jan Kara wrote:
> > In quota code we need to find a superblock corresponding to a device and wait
> > for superblock to be unfrozen. However this waiting has to happen without
> > s_umount semaphore because that is required for superblock to thaw. So provide
> > a function in VFS for this to keep dances with s_umount where they belong.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
> >  include/linux/fs.h |    1 +
> >  2 files changed, 36 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 6015c02..e320239 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
> >  
> >  EXPORT_SYMBOL(iterate_supers_type);
> >  
> > -/**
> > - *	get_super - get the superblock of a device
> > - *	@bdev: device to get the superblock for
> > - *	
> > - *	Scans the superblock list and finds the superblock of the file system
> > - *	mounted on the device given. %NULL is returned if no match is found.
> > - */
> > -
> > -struct super_block *get_super(struct block_device *bdev)
> > +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
> >  {
> >  	struct super_block *sb;
> >  
> > @@ -618,9 +610,13 @@ rescan:
> >  			spin_unlock(&sb_lock);
> >  			down_read(&sb->s_umount);
> >  			/* still alive? */
> > -			if (sb->s_root && (sb->s_flags & MS_BORN))
> > -				return sb;
> > -			up_read(&sb->s_umount);
> > +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> > +				if (sb->s_frozen == SB_UNFROZEN)
> > +					return sb;
> > +				up_read(&sb->s_umount);
> > +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> > +			} else
> > +				up_read(&sb->s_umount);
> >  			/* nope, got unmounted */
> >  			spin_lock(&sb_lock);
> >  			__put_super(sb);
> > @@ -631,9 +627,36 @@ rescan:
> >  	return NULL;
> >  }
> 
> The thawed parameter appears to be unused and the waiting for frozen
> filesystems appears to happen for all callers. Is this intentional?
  Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
up.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

On Thu 09-02-12 00:27:05, Jan Kara wrote:
> On Thu 09-02-12 10:20:49, Dave Chinner wrote:
> > On Tue, Feb 07, 2012 at 11:37:46PM +0100, Jan Kara wrote:
> > > In quota code we need to find a superblock corresponding to a device and wait
> > > for superblock to be unfrozen. However this waiting has to happen without
> > > s_umount semaphore because that is required for superblock to thaw. So provide
> > > a function in VFS for this to keep dances with s_umount where they belong.
> > > 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
> > >  include/linux/fs.h |    1 +
> > >  2 files changed, 36 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/fs/super.c b/fs/super.c
> > > index 6015c02..e320239 100644
> > > --- a/fs/super.c
> > > +++ b/fs/super.c
> > > @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
> > >  
> > >  EXPORT_SYMBOL(iterate_supers_type);
> > >  
> > > -/**
> > > - *	get_super - get the superblock of a device
> > > - *	@bdev: device to get the superblock for
> > > - *	
> > > - *	Scans the superblock list and finds the superblock of the file system
> > > - *	mounted on the device given. %NULL is returned if no match is found.
> > > - */
> > > -
> > > -struct super_block *get_super(struct block_device *bdev)
> > > +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
> > >  {
> > >  	struct super_block *sb;
> > >  
> > > @@ -618,9 +610,13 @@ rescan:
> > >  			spin_unlock(&sb_lock);
> > >  			down_read(&sb->s_umount);
> > >  			/* still alive? */
> > > -			if (sb->s_root && (sb->s_flags & MS_BORN))
> > > -				return sb;
> > > -			up_read(&sb->s_umount);
> > > +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> > > +				if (sb->s_frozen == SB_UNFROZEN)
> > > +					return sb;
> > > +				up_read(&sb->s_umount);
> > > +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> > > +			} else
> > > +				up_read(&sb->s_umount);
> > >  			/* nope, got unmounted */
> > >  			spin_lock(&sb_lock);
> > >  			__put_super(sb);
> > > @@ -631,9 +627,36 @@ rescan:
> > >  	return NULL;
> > >  }
> > 
> > The thawed parameter appears to be unused and the waiting for frozen
> > filesystems appears to happen for all callers. Is this intentional?
>   Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
> up.
  Attached is a fixed version.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-vfs-Provide-function-to-get-superblock-and-wait-for-.patch --]
[-- Type: text/x-patch, Size: 3554 bytes --]

>From 5dce7adb0c281612a14fa3dd8c8d5ef3f5eb3666 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 7 Feb 2012 22:59:06 +0100
Subject: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

In quota code we need to find a superblock corresponding to a device and wait
for superblock to be unfrozen. However this waiting has to happen without
s_umount semaphore because that is required for superblock to thaw. So provide
a function in VFS for this to keep dances with s_umount where they belong.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |    1 +
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 6015c02..e15aaa9 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
 
 EXPORT_SYMBOL(iterate_supers_type);
 
-/**
- *	get_super - get the superblock of a device
- *	@bdev: device to get the superblock for
- *	
- *	Scans the superblock list and finds the superblock of the file system
- *	mounted on the device given. %NULL is returned if no match is found.
- */
-
-struct super_block *get_super(struct block_device *bdev)
+static struct super_block *__get_super(struct block_device *bdev, bool thawed)
 {
 	struct super_block *sb;
 
@@ -618,9 +610,13 @@ rescan:
 			spin_unlock(&sb_lock);
 			down_read(&sb->s_umount);
 			/* still alive? */
-			if (sb->s_root && (sb->s_flags & MS_BORN))
-				return sb;
-			up_read(&sb->s_umount);
+			if (sb->s_root && (sb->s_flags & MS_BORN)) {
+				if (!thawed || sb->s_frozen == SB_UNFROZEN)
+					return sb;
+				up_read(&sb->s_umount);
+				vfs_check_frozen(sb, SB_FREEZE_WRITE);
+			} else
+				up_read(&sb->s_umount);
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
 			__put_super(sb);
@@ -631,9 +627,36 @@ rescan:
 	return NULL;
 }
 
+/**
+ *	get_super - get the superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device given. %NULL is returned if no match is found.
+ */
+
+struct super_block *get_super(struct block_device *bdev)
+{
+	return __get_super(bdev, false);
+}
 EXPORT_SYMBOL(get_super);
 
 /**
+ *	get_super_thawed - get thawed superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device given once the superblock is thawed. %NULL is
+ *	returned if no match is found.
+ */
+
+struct super_block *get_super_thawed(struct block_device *bdev)
+{
+	return __get_super(bdev, true);
+}
+EXPORT_SYMBOL(get_super_thawed);
+
+/**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
  *
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 386da09..69cd5bb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2496,6 +2496,7 @@ extern void get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
+extern struct super_block *get_super_thawed(struct block_device *);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
-- 
1.7.1

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Eric Sandeen]

On 2/8/12 5:47 PM, Jan Kara wrote:
> On Thu 09-02-12 00:27:05, Jan Kara wrote:
>> On Thu 09-02-12 10:20:49, Dave Chinner wrote:

...

>>> The thawed parameter appears to be unused and the waiting for frozen
>>> filesystems appears to happen for all callers. Is this intentional?
>>   Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
>> up.
>   Attached is a fixed version.
> 
> 								Honza

> From 5dce7adb0c281612a14fa3dd8c8d5ef3f5eb3666 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Tue, 7 Feb 2012 22:59:06 +0100
> Subject: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw
> 
> In quota code we need to find a superblock corresponding to a device and wait
> for superblock to be unfrozen. However this waiting has to happen without
> s_umount semaphore because that is required for superblock to thaw. So provide
> a function in VFS for this to keep dances with s_umount where they belong.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
>  include/linux/fs.h |    1 +
>  2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 6015c02..e15aaa9 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
>  
>  EXPORT_SYMBOL(iterate_supers_type);
>  
> -/**
> - *	get_super - get the superblock of a device
> - *	@bdev: device to get the superblock for
> - *	
> - *	Scans the superblock list and finds the superblock of the file system
> - *	mounted on the device given. %NULL is returned if no match is found.
> - */

> -
> -struct super_block *get_super(struct block_device *bdev)
> +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
>  {
>  	struct super_block *sb;
>  
> @@ -618,9 +610,13 @@ rescan:
>  			spin_unlock(&sb_lock);
>  			down_read(&sb->s_umount);
>  			/* still alive? */
> -			if (sb->s_root && (sb->s_flags & MS_BORN))
> -				return sb;
> -			up_read(&sb->s_umount);
> +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> +				if (!thawed || sb->s_frozen == SB_UNFROZEN)

I guess this has the same race we are worried about elsewhere (sb gets
frozen immediately after we check) but probably can't fix that yet.

would "wait_for_thaw" vs. "thaw" be any clearer?  Nitpicky I guess but the
meaning of "thawed" isn't immediately clear here.  If it's already thawed?  If
we want to wait for it it to be thawed?  You can figure it out from the callers
but maybe a comment or a different name might help.  No big deal.
 
> +					return sb;
> +				up_read(&sb->s_umount);
> +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> +			} else
> +				up_read(&sb->s_umount);
>  			/* nope, got unmounted */
>  			spin_lock(&sb_lock);
>  			__put_super(sb);
> @@ -631,9 +627,36 @@ rescan:
>  	return NULL;
>  }
>  
> +/**
> + *	get_super - get the superblock of a device
> + *	@bdev: device to get the superblock for
> + *
> + *	Scans the superblock list and finds the superblock of the file system
> + *	mounted on the device given. %NULL is returned if no match is found.
> + */

I think it'd be nice to explicitly say in the comment that this may
return a frozen superblock.

> +struct super_block *get_super(struct block_device *bdev)
> +{
> +	return __get_super(bdev, false);
> +}
>  EXPORT_SYMBOL(get_super);
>  
>  /**
> + *	get_super_thawed - get thawed superblock of a device
> + *	@bdev: device to get the superblock for
> + *
> + *	Scans the superblock list and finds the superblock of the file system
> + *	mounted on the device given once the superblock is thawed. %NULL is
> + *	returned if no match is found.
> + */

And to explicitly say that this one will not return until the sb is unfrozen?

Otherwise, this seems fine to me, thanks.

-Eric

> +struct super_block *get_super_thawed(struct block_device *bdev)
> +{
> +	return __get_super(bdev, true);
> +}
> +EXPORT_SYMBOL(get_super_thawed);
> +
> +/**
>   * get_active_super - get an active reference to the superblock of a device
>   * @bdev: device to get the superblock for
>   *
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 386da09..69cd5bb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2496,6 +2496,7 @@ extern void get_filesystem(struct file_system_type *fs);
>  extern void put_filesystem(struct file_system_type *fs);
>  extern struct file_system_type *get_fs_type(const char *name);
>  extern struct super_block *get_super(struct block_device *);
> +extern struct super_block *get_super_thawed(struct block_device *);
>  extern struct super_block *get_active_super(struct block_device *bdev);
>  extern void drop_super(struct super_block *sb);
>  extern void iterate_supers(void (*)(struct super_block *, void *), void *);

Re: [PATCH 2/2] quota: Fix deadlock with suspend and quotas

[Eric Sandeen]

On 2/7/12 4:37 PM, Jan Kara wrote:
> This script causes a kernel deadlock:
> set -e
> DEVICE=/dev/vg1/linear
> lvchange -ay $DEVICE
> mkfs.ext3 $DEVICE
> mount -t ext3 -o usrquota,grpquota $DEVICE /mnt/test
> quotacheck -gu /mnt/test
> umount /mnt/test
> mount -t ext3 -o usrquota,grpquota $DEVICE /mnt/test
> quotaon /mnt/test
> dmsetup suspend $DEVICE
> setquota -u root 1 2 3 4 /mnt/test &
> sleep 1
> dmsetup resume $DEVICE
> 
> setquota acquired semaphore s_umount for read and then tried to perform a
> transaction (and waits because the device is suspended).  dmsetup resume tries
> to acquire s_umount for write before resuming the device (and waits for
> setquota).
> 
> Fix the deadlock by grabbing a thawed superblock for quota commands which need
> it.
> 
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Makes sense to me, thanks.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/quota/quota.c |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 7898cd6..fc2c438 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -292,11 +292,26 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
>  	}
>  }
>  
> +/* Return 1 if 'cmd' will block on frozen filesystem */
> +static int quotactl_cmd_write(int cmd)
> +{
> +	switch (cmd) {
> +	case Q_GETFMT:
> +	case Q_GETINFO:
> +	case Q_SYNC:
> +	case Q_XGETQSTAT:
> +	case Q_XGETQUOTA:
> +	case Q_XQUOTASYNC:
> +		return 0;
> +	}
> +	return 1;
> +}
> +
>  /*
>   * look up a superblock on which quota ops will be performed
>   * - use the name of a block device to find the superblock thereon
>   */
> -static struct super_block *quotactl_block(const char __user *special)
> +static struct super_block *quotactl_block(const char __user *special, int cmd)
>  {
>  #ifdef CONFIG_BLOCK
>  	struct block_device *bdev;
> @@ -309,7 +324,10 @@ static struct super_block *quotactl_block(const char __user *special)
>  	putname(tmp);
>  	if (IS_ERR(bdev))
>  		return ERR_CAST(bdev);
> -	sb = get_super(bdev);
> +	if (quotactl_cmd_write(cmd))
> +		sb = get_super_thawed(bdev);
> +	else
> +		sb = get_super(bdev);
>  	bdput(bdev);
>  	if (!sb)
>  		return ERR_PTR(-ENODEV);
> @@ -361,7 +379,7 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special,
>  			pathp = &path;
>  	}
>  
> -	sb = quotactl_block(special);
> +	sb = quotactl_block(special, cmds);
>  	if (IS_ERR(sb)) {
>  		ret = PTR_ERR(sb);
>  		goto out;

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Mikulas Patocka]

On Thu, 9 Feb 2012, Eric Sandeen wrote:

> On 2/8/12 5:47 PM, Jan Kara wrote:
> > On Thu 09-02-12 00:27:05, Jan Kara wrote:
> >> On Thu 09-02-12 10:20:49, Dave Chinner wrote:
> 
> ...
> 
> >>> The thawed parameter appears to be unused and the waiting for frozen
> >>> filesystems appears to happen for all callers. Is this intentional?
> >>   Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
> >> up.
> >   Attached is a fixed version.
> > 
> > 								Honza
> 
> > From 5dce7adb0c281612a14fa3dd8c8d5ef3f5eb3666 Mon Sep 17 00:00:00 2001
> > From: Jan Kara <jack@suse.cz>
> > Date: Tue, 7 Feb 2012 22:59:06 +0100
> > Subject: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw
> > 
> > In quota code we need to find a superblock corresponding to a device and wait
> > for superblock to be unfrozen. However this waiting has to happen without
> > s_umount semaphore because that is required for superblock to thaw. So provide
> > a function in VFS for this to keep dances with s_umount where they belong.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
> >  include/linux/fs.h |    1 +
> >  2 files changed, 36 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 6015c02..e15aaa9 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
> >  
> >  EXPORT_SYMBOL(iterate_supers_type);
> >  
> > -/**
> > - *	get_super - get the superblock of a device
> > - *	@bdev: device to get the superblock for
> > - *	
> > - *	Scans the superblock list and finds the superblock of the file system
> > - *	mounted on the device given. %NULL is returned if no match is found.
> > - */
> 
> > -
> > -struct super_block *get_super(struct block_device *bdev)
> > +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
> >  {
> >  	struct super_block *sb;
> >  
> > @@ -618,9 +610,13 @@ rescan:
> >  			spin_unlock(&sb_lock);
> >  			down_read(&sb->s_umount);
> >  			/* still alive? */
> > -			if (sb->s_root && (sb->s_flags & MS_BORN))
> > -				return sb;
> > -			up_read(&sb->s_umount);
> > +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> > +				if (!thawed || sb->s_frozen == SB_UNFROZEN)
> 
> I guess this has the same race we are worried about elsewhere (sb gets
> frozen immediately after we check) but probably can't fix that yet.

It doesn't --- s_umount is locked for read for the whole duration of the 
quota call, so superblock cannot be frozen (because freezing takes 
s_umount for write).

Mikulas

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

On Thu 09-02-12 10:37:20, Eric Sandeen wrote:
> On 2/8/12 5:47 PM, Jan Kara wrote:
> > On Thu 09-02-12 00:27:05, Jan Kara wrote:
> >> On Thu 09-02-12 10:20:49, Dave Chinner wrote:
> >>> The thawed parameter appears to be unused and the waiting for frozen
> >>> filesystems appears to happen for all callers. Is this intentional?
> >>   Ah, sorry. Obviously that's a bug. Thanks for spotting it. I'll fix it
> >> up.
> >   Attached is a fixed version.
> > 
> > 								Honza
> 
> > From 5dce7adb0c281612a14fa3dd8c8d5ef3f5eb3666 Mon Sep 17 00:00:00 2001
> > From: Jan Kara <jack@suse.cz>
> > Date: Tue, 7 Feb 2012 22:59:06 +0100
> > Subject: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw
> > 
> > In quota code we need to find a superblock corresponding to a device and wait
> > for superblock to be unfrozen. However this waiting has to happen without
> > s_umount semaphore because that is required for superblock to thaw. So provide
> > a function in VFS for this to keep dances with s_umount where they belong.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/super.c         |   47 +++++++++++++++++++++++++++++++++++------------
> >  include/linux/fs.h |    1 +
> >  2 files changed, 36 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 6015c02..e15aaa9 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -593,15 +593,7 @@ void iterate_supers_type(struct file_system_type *type,
> >  
> >  EXPORT_SYMBOL(iterate_supers_type);
> >  
> > -/**
> > - *	get_super - get the superblock of a device
> > - *	@bdev: device to get the superblock for
> > - *	
> > - *	Scans the superblock list and finds the superblock of the file system
> > - *	mounted on the device given. %NULL is returned if no match is found.
> > - */
> 
> > -
> > -struct super_block *get_super(struct block_device *bdev)
> > +static struct super_block *__get_super(struct block_device *bdev, bool thawed)
> >  {
> >  	struct super_block *sb;
> >  
> > @@ -618,9 +610,13 @@ rescan:
> >  			spin_unlock(&sb_lock);
> >  			down_read(&sb->s_umount);
> >  			/* still alive? */
> > -			if (sb->s_root && (sb->s_flags & MS_BORN))
> > -				return sb;
> > -			up_read(&sb->s_umount);
> > +			if (sb->s_root && (sb->s_flags & MS_BORN)) {
> > +				if (!thawed || sb->s_frozen == SB_UNFROZEN)
> 
> I guess this has the same race we are worried about elsewhere (sb gets
> frozen immediately after we check) but probably can't fix that yet.
  As Mikulas said, this is OK. We return to the caller with s_umount held
thus freezing cannot happen in parallel. That's why I'm sending this as a
separate patch set...

> would "wait_for_thaw" vs. "thaw" be any clearer?  Nitpicky I guess but the
> meaning of "thawed" isn't immediately clear here.  If it's already thawed?  If
> we want to wait for it it to be thawed?  You can figure it out from the callers
> but maybe a comment or a different name might help.  No big deal.
  How about "wait_thaw". That should be explicit enough...

>  
> > +					return sb;
> > +				up_read(&sb->s_umount);
> > +				vfs_check_frozen(sb, SB_FREEZE_WRITE);
> > +			} else
> > +				up_read(&sb->s_umount);
> >  			/* nope, got unmounted */
> >  			spin_lock(&sb_lock);
> >  			__put_super(sb);
> > @@ -631,9 +627,36 @@ rescan:
> >  	return NULL;
> >  }
> >  
> > +/**
> > + *	get_super - get the superblock of a device
> > + *	@bdev: device to get the superblock for
> > + *
> > + *	Scans the superblock list and finds the superblock of the file system
> > + *	mounted on the device given. %NULL is returned if no match is found.
> > + */
> 
> I think it'd be nice to explicitly say in the comment that this may
> return a frozen superblock.
  Will do.

> > +struct super_block *get_super(struct block_device *bdev)
> > +{
> > +	return __get_super(bdev, false);
> > +}
> >  EXPORT_SYMBOL(get_super);
> >  
> >  /**
> > + *	get_super_thawed - get thawed superblock of a device
> > + *	@bdev: device to get the superblock for
> > + *
> > + *	Scans the superblock list and finds the superblock of the file system
> > + *	mounted on the device given once the superblock is thawed. %NULL is
> > + *	returned if no match is found.
> > + */
> 
> And to explicitly say that this one will not return until the sb is unfrozen?
  It is written in the comment ("once the superblock is thawed"). But the
sentence looks ugly when I read it now so I'll try to reformulate it.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Dave Chinner]

On Fri, Feb 10, 2012 at 10:48:58AM +0100, Jan Kara wrote:
> On Thu 09-02-12 10:37:20, Eric Sandeen wrote:
> > would "wait_for_thaw" vs. "thaw" be any clearer?  Nitpicky I guess but the
> > meaning of "thawed" isn't immediately clear here.  If it's already thawed?  If
> > we want to wait for it it to be thawed?  You can figure it out from the callers
> > but maybe a comment or a different name might help.  No big deal.
>   How about "wait_thaw". That should be explicit enough...

IMO, wait_for_thaw is much easier to read (i.e. better english ;) and
hence the code documents itself better. There's no reason to skimp
on characters here - it's not a heavily used variable....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

On Sun 12-02-12 21:13:25, Al Viro wrote:
> On Fri, Feb 10, 2012 at 11:03:00AM +0100, Jan Kara wrote:
> > In quota code we need to find a superblock corresponding to a device and wait
> > for superblock to be unfrozen. However this waiting has to happen without
> > s_umount semaphore because that is required for superblock to thaw. So provide
> > a function in VFS for this to keep dances with s_umount where they belong.
> 
> Eww...  All it takes is
> struct super_block *get_super_thawed(struct block_device *bdev)
> {
>         while (1) {
>                 struct super_block *s = get_super(bdev);
>                 if (!s || s->s_frozen == SB_UNFROZEN)
>                         return s;
> 		up_read(&s->s_umount);
> 		vfs_check_frozen(s, SB_FREEZE_WRITE);
> 		put_super(s);
>         }
> }
> and there's no need of extra arguments, etc. whatsoever.  Both patches
> applied, with implementation of get_super_thawed() done as above.
  Yeah, this looks better. Thanks!

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Al Viro]

On Fri, Feb 10, 2012 at 11:03:00AM +0100, Jan Kara wrote:
> In quota code we need to find a superblock corresponding to a device and wait
> for superblock to be unfrozen. However this waiting has to happen without
> s_umount semaphore because that is required for superblock to thaw. So provide
> a function in VFS for this to keep dances with s_umount where they belong.

Eww...  All it takes is
struct super_block *get_super_thawed(struct block_device *bdev)
{
        while (1) {
                struct super_block *s = get_super(bdev);
                if (!s || s->s_frozen == SB_UNFROZEN)
                        return s;
		up_read(&s->s_umount);
		vfs_check_frozen(s, SB_FREEZE_WRITE);
		put_super(s);
        }
}
and there's no need of extra arguments, etc. whatsoever.  Both patches
applied, with implementation of get_super_thawed() done as above.

[PATCH 1/2] vfs: Provide function to get superblock and wait for it to thaw

[Jan Kara]

In quota code we need to find a superblock corresponding to a device and wait
for superblock to be unfrozen. However this waiting has to happen without
s_umount semaphore because that is required for superblock to thaw. So provide
a function in VFS for this to keep dances with s_umount where they belong.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/super.c         |   50 ++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |    1 +
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 6015c02..1f657d6 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -593,15 +593,8 @@ void iterate_supers_type(struct file_system_type *type,
 
 EXPORT_SYMBOL(iterate_supers_type);
 
-/**
- *	get_super - get the superblock of a device
- *	@bdev: device to get the superblock for
- *	
- *	Scans the superblock list and finds the superblock of the file system
- *	mounted on the device given. %NULL is returned if no match is found.
- */
-
-struct super_block *get_super(struct block_device *bdev)
+static struct super_block *__get_super(struct block_device *bdev,
+				       bool wait_thaw)
 {
 	struct super_block *sb;
 
@@ -618,9 +611,13 @@ rescan:
 			spin_unlock(&sb_lock);
 			down_read(&sb->s_umount);
 			/* still alive? */
-			if (sb->s_root && (sb->s_flags & MS_BORN))
-				return sb;
-			up_read(&sb->s_umount);
+			if (sb->s_root && (sb->s_flags & MS_BORN)) {
+				if (!wait_thaw || sb->s_frozen == SB_UNFROZEN)
+					return sb;
+				up_read(&sb->s_umount);
+				vfs_check_frozen(sb, SB_FREEZE_WRITE);
+			} else
+				up_read(&sb->s_umount);
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
 			__put_super(sb);
@@ -631,9 +628,38 @@ rescan:
 	return NULL;
 }
 
+/**
+ *	get_super - get the superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device given. %NULL is returned if no match is found.
+ *	Note that the returned superblock may be frozen.
+ */
+
+struct super_block *get_super(struct block_device *bdev)
+{
+	return __get_super(bdev, false);
+}
 EXPORT_SYMBOL(get_super);
 
 /**
+ *	get_super_thawed - get thawed superblock of a device
+ *	@bdev: device to get the superblock for
+ *
+ *	Scans the superblock list and finds the superblock of the file system
+ *	mounted on the device. The superblock is returned once it is thawed
+ *	(or immediately if it was not frozen). %NULL is returned if no match
+ *	is found.
+ */
+
+struct super_block *get_super_thawed(struct block_device *bdev)
+{
+	return __get_super(bdev, true);
+}
+EXPORT_SYMBOL(get_super_thawed);
+
+/**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
  *
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 386da09..69cd5bb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2496,6 +2496,7 @@ extern void get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
+extern struct super_block *get_super_thawed(struct block_device *);
 extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
-- 
1.7.1