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

[Jan Kara <jack@suse.cz>]

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