Re: [RFC][PATCH 04/36] btrfs: Add checkpoint file operations

[Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

On Mon, May 04, 2009 at 05:25:13PM -0500, Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> > 
> > Serge E. Hallyn wrote:
> > > Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> > > 
> > > ...
> > > 
> > >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > >> index a7acfe6..f16be9d 100644
> > >> --- a/fs/btrfs/super.c
> > >> +++ b/fs/btrfs/super.c
> > >> @@ -686,6 +686,7 @@ static const struct file_operations btrfs_ctl_fops = {
> > >>  	.unlocked_ioctl	 = btrfs_control_ioctl,
> > >>  	.compat_ioctl = btrfs_control_ioctl,
> > >>  	.owner	 = THIS_MODULE,
> > >> +	.checkpoint = generic_file_checkpoint,
> > >>  };
> > > 
> > > Do we really want this one?
> > 
> > We need to checkpoint the open file regardless of whether the underlying
> > filesystem has snapshot capabilities.
> 
> But maybe we need to refuse a checkpoint if a task is talking to the
> underlying btrfs filesystem?

Good catch. The ctl file is a device node which, as best I can tell, is
empty. So the only obvious concern are the unlocked_ioctl()s. I expect
these ioctls() are "special" so generic_file_checkpoint() is probably
insufficient here.

v2 of the btrfs checkpoint op patch, which removes this hunk, is below.

A non-generic handler which blocks or prevents ioctl() during checkpoint would
seem to be required. Definitely a topic worthy of discussion on linux-fsdevel if
/when we add a .checkpoint op for this. 

As best I can tell we don't have anything to handle the generic ioctl() case
either. Am I wrong?

Cheers,
	-Matt Helsley
---

Add the checkpoint operation for btrfs files and directories.


Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 fs/btrfs/file.c  |    1 +
 fs/btrfs/inode.c |    1 +
 fs/btrfs/super.c |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 482f8db..23d5c72 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1334,4 +1334,5 @@ struct file_operations btrfs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= btrfs_ioctl,
 #endif
+	.checkpoint	= generic_file_checkpoint,
 };
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 65219f6..583d57e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5158,6 +5158,7 @@ static struct file_operations btrfs_dir_file_operations = {
 #endif
 	.release        = btrfs_release_file,
 	.fsync		= btrfs_sync_file,
+	.checkpoint	= generic_file_checkpoint,
 };

 static struct extent_io_ops btrfs_extent_io_ops = {