RE: [RFC PATCH] fs: file freeze support

[Namjae Jeon <namjae.jeon@samsung.com>]

>   Hello,
Hi Jan.
> 
> On Fri 16-01-15 15:48:04, Namjae Jeon wrote:
> > > > +int file_write_unfreeze(struct inode *inode)
> > > > +{
> > > > +	struct super_block *sb = inode->i_sb;
> > > > +
> > > > +	if (!S_ISREG(inode->i_mode))
> > > > +		return -EINVAL;
> > > > +
> > > > +	spin_lock(&inode->i_lock);
> > > > +
> > > > +	if (!(inode->i_state & I_WRITE_FREEZED)) {
> > > > +		spin_unlock(&inode->i_lock);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	inode->i_state &= ~I_WRITE_FREEZED;
> > > > +	smp_wmb();
> > > > +	wake_up(&sb->s_writers.wait_unfrozen);
> > > > +	spin_unlock(&inode->i_lock);
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(file_write_unfreeze);
> > >   So I was looking at the implementation and I have a few comments:
> > > 1) The trick with freezing superblock looks nice but I'm somewhat worried
> > > that if we wanted to heavily use per-inode freezing to defrag the whole
> > > filesystem it may be too slow to freeze the whole fs, mark one inode as
> > > frozen and then unfreeze the fs. But I guess we'll see that once have some
> > > reasonably working implementation.
> > Dmitry has given a good idea to avoid multiple freeze fs and unfreeze fs
> > calls.
> >
> > ioctl(sb,FIFREEZE)
> > while (f = pop(files_list))
> >   ioctl(f,FS_IOC_FWFREEZE)
> > ioctl(sb,FITHAW)
> >
> > In file_write_freeze, we could first check if the fs is already frozen,
> > if yes than we can directly set inode write freeze state after taking
> > relevant lock to prevent fs_thaw while the inode state is being set.
>   Well, doing fs-wide freezing from userspace makes sense as Dmitry pointed
> out. We can then just fail FS_IOC_FWFREEZE with error when the whole fs isn't
> frozen. I'm just somewhat worried whether the fs-wide freezing won't be too
> fragile. E.g. consider a situation when you are running a defrag program
> which is freezing and unfreezing the filesystem and then some background
> work kicks which will want to snapshot the filesystem so it will freeze &
> unfreeze the fs as well. Now depending on how exactly defrag and snapshot
> race one of the FIFREEZE ioctls will return EBUSY and the process
> (hopefully gracefully) fails.
> 
> This isn't a new situation - if you ran two snapshots at once, you'd see
> the same failure. But the more fs-wide freezing gets used in different
> places the stranger and less expected failure you'll see...
Yes, Right. Thanks for your opinion. I will consider your point.

> 
> > > 2) The tests you are currently doing are racy. If
> > > things happen as:
> > >   CPU1					CPU2
> > > inode_start_write()
> > > 					file_write_freeze()
> > > sb_start_pagefault()
> > > Do modifications.
> > >
> > > Then you have a CPU modifying a file while file_write_freeze() has
> > > succeeded so it should be frozen.
> > >
> > > If you swap inode_start_write() with sb_start_pagefault() the above race
> > > doesn't happen but userspace program has to be really careful not to hit a
> > > deadlock. E.g. if you tried to freeze two inodes the following could happen:
> > >   CPU1					CPU2
> > > 					file_write_freeze(inode1)
> > > fault on inode1:
> > > sb_start_pagefault()
> > > inode_start_write() -> blocks
> > > 					file_write_freeze(inode2)
> > > 					  blocks in freeze_super()
> > >
> > > So I don't think this is a good scheme for inode freezing...
> > To solve this race, we can fold inode_start_write with sb_start_write and use
> > similar appraoch of __sb_start_write.
> > How about the below scheme ?
> >
> > void inode_start_write(struct inode *inode)
> > {
> > 	struct super_block *sb = inode->i_sb;
> >
> > retry:
> >
> > 	if (unlikely(inode->i_state & I_WRITE_FREEZED)) {
> > 		DEFINE_WAIT(wait);
> >
> > 		prepare_to_wait(&sb->s_writers.wait_unfrozen, &wait,
> > 			TASK_UNINTERRUPTIBLE);
> > 		schedule();
> > 		finish_wait(&sb->s_writers.wait_unfrozen, &wait);
> >
> > 		goto retry;
> > 	}
> >
> > 	sb_start_write(sb);
> >
> > 	/* check if file_write_freeze race with us */
> > 	if (unlikely(inode->i_state & I_WRITE_FREEZED) {
> > 		sb_end_write(sb);
> > 		goto retry;
> > 	}
> > }
>   Yes, this should work. You'll need a similar wrapper for page faults but
> that's easy enough.
Okay, Thanks :)
> 
> 								Honza
> 
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR