Messages in this thread | | | From | "Takashi Sato" <> | Subject | Re: [PATCH 1/3] Implement generic freeze feature | Date | Thu, 11 Sep 2008 19:58:08 +0900 |
| |
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
| |