[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[dm-devel] Re: [PATCH 1/3] Implement generic freeze feature



Hi,

Eric Sandeen:
+static int ioctl_freeze(struct file *filp)
+{
+     struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
+
+     if (!capable(CAP_SYS_ADMIN))
+         return -EPERM;
+
+     /* If filesystem doesn't support freeze feature, return. */
+     if (sb->s_op->write_super_lockfs == NULL)
+         return -EOPNOTSUPP;
+
+     /* If a regular file or a directory isn't specified, return. */
+     if (sb->s_bdev == NULL)
+         return -EINVAL;
+
+     /* Freeze */
+     sb = freeze_bdev(sb->s_bdev);
+     if (IS_ERR(sb))
+         return PTR_ERR(sb);
+     return 0;
+}

Not a problem with your patch exactly, but I was just wondering; you
check here whether the sb returned from freeze_bdev is an ERR_PTR (as
does lock_fs()) - but, freeze_bdev never returns an error, does it?
->write_super_lockfs is a void...

It really seems that at least we should be able to handle IO errors on
the freeze request, and tell the user "No, your filesystem was not
frozen..."?

Maybe I'll whip up a patch to see about propagating freeze errors up
from the filesystems that implement it, unless I'm missing some reason
not to do so...?

Right.
We should handle an IO error which occurs in write_super_lockfs.
I will change the write_super_lockfs's type to "int" so that it can return an error.
And I will consider returning an error of ext3_write_super_lockfs because
journal_flush() in ext3_write_super_lockfs() doesn't handle an IO error.

Also, should this be checking for a NULL returned from freeze_bdev as
well?  I guess this should never happen if we have a file open on which
we are calling the ioctl ...

I think ioctl_freeze doesn't need to check NULL because it never happen
as you said.

Cheers, Takashi


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]