Re: [PATCH 3/3] Add timeout feature

[Takashi Sato]

Hi,

> > o Reset the timeout period
> >   int ioctl(int fd, int FIFREEZE_RESET_TIMEOUT, long *timeval)
> >     fd:file descriptor of mountpoint
> >     FIFREEZE_RESET_TIMEOUT: request code for reset of timeout period
> >     timeval: new timeout period in seconds
> >     Return value: 0 if the operation succeeds. Otherwise, -1
> >     Error number: If the filesystem has already been unfrozen,
> >                   errno is set to EINVAL.
>
> Please avoid the use of the term `timeval' here.  That term is closely
> associated with `struct timeval', and these are not being used here.  A
> better identifier would be timeout_secs - it tells the reader what it
> does, and it tells the reader what units it is in.  And when it comes
> to "time", it is very useful to tell people which units are being used,
> because there are many different ones.

OK.
I will use "timeout_secs" in the next version to match the source code.

> > -static int ioctl_freeze(struct file *filp)
> > +static int ioctl_freeze(struct file *filp, unsigned long arg)
> >  {
> > +    long timeout_sec;
> > +    long timeout_msec;
> >      struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> > +    int error;
> >
> >      if (!capable(CAP_SYS_ADMIN))
> >          return -EPERM;
> > @@ -159,8 +163,27 @@ static int ioctl_freeze(struct file *fil
> >      if (sb->s_op->write_super_lockfs == NULL)
> >          return -EOPNOTSUPP;
> >
> > +    /* arg(sec) to tick value. */
> > +    error = get_user(timeout_sec, (long __user *) arg);>
> uh-oh, compat problems.
>
> A 32-bit application running under a 32-bit kernel will need to provide
> a 32-bit quantity.
>
> A 32-bit application running under a 64-bit kernel will need to provide
> a 64-bit quantity.
>
> I suggest that you go through the entire patch and redo this part of
> the kernel ABI in terms of __u32, and avoid any mention of "long" in
> the kernel<->userspace interface.

I agree.
I will replace long with a 32bit integer type.

> > +    /* arg(sec) to tick value. */
> > +    error = get_user(timeout_sec, (long __user *) arg);
> > +    if (timeout_sec < 0)
> > +        return -EINVAL;
> > +    else if (timeout_sec < 2)
> > +        timeout_sec = 0;> The `else' is unneeded.
>
> It would be clearer to code this all as:
>
> if (timeout_sec < 0)
>     return -EINVAL;
>
> if (timeout_sec == 1) {
>     /*
>      * If 1 is specified as the timeout period it is changed into 0
>      * to retain compatibility with XFS's xfs_freeze.
>      */
>     timeout_sec = 0;
> }

OK.

> > +    if (error)
> > +        return error;
> > +    timeout_msec = timeout_sec * 1000;
> > +    if (timeout_msec < 0)
> > +        return -EINVAL;
>
> um, OK, but timeout_msec could have overflowed during the
> multiplication and could be complete garbage.  I guess it doesn't
> matter much, but a cleaner approach might be:
>
> if (timeout_sec > LONG_MAX/1000)
>     return -EINVAL;
>
> or something like that.

OK.

> But otoh, why do the multiplication at all?  If we're able to handle
> the timeout down to the millisecond level, why not offer that
> resolution to userspace?  Offer the increased time granularity to the
> application?

I think there is no case the user specifies it in millisecond,
so the second granularity is more comfortable.

> > +    if (sb) {
>
> Can this happen?

I have found that sb == NULL never happen
but sb->s_bdev == NULL can happen when we call this ioctl
for a device file (not a directory or a regular file).
So I will add the following check.
if (sb->s_bdev == NULL)
   return -EINVAL;

> > +    if (test_and_set_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state))
> > +        return -EBUSY;
> > +    if (sb->s_frozen == SB_UNFROZEN) {
> > +        clear_bit(BD_FREEZE_OP, &sb->s_bdev->bd_state);
> > +        return -EINVAL;
> > +    }
> > +    /* setup unfreeze timer */
> > +    if (timeout_msec > 0)
>
> What does this test do?  afacit it's special-casing the timeout_secs==0
> case.  Was this feature documented?  What does it do?

There is no special case of timeout_secs==0.
I will remove this check.

> > +void add_freeze_timeout(struct block_device *bdev, long timeout_msec)
> > +{
> > +    s64 timeout_jiffies = msecs_to_jiffies(timeout_msec);
> > +
> > +    /* Set delayed work queue */
> > +    cancel_delayed_work(&bdev->bd_freeze_timeout);
>
> Should this have used cancel_delayed_work_sync()?

Exactly.
I will replace cancel_delayed_work with cancel_delayed_work_sync.

> > +void del_freeze_timeout(struct block_device *bdev)
> > +{
> > + if (delayed_work_pending(&bdev->bd_freeze_timeout))
>
> Is this test needed?

It's not necessary because cancel_delayed_work_sync checks it itself.
I will remove it.

> > --- linux-2.6.26-rc7-xfs/fs/xfs/xfs_fsops.c 2008-06-23 11:55:15.000000000 +0900
> > +++ linux-2.6.26-rc7-timeout/fs/xfs/xfs_fsops.c 2008-06-23 11:56:47.000000000 +0900
> > @@ -619,7 +619,7 @@ xfs_fs_goingdown(
> >  {
> >  switch (inflags) {
> >  case XFS_FSOP_GOING_FLAGS_DEFAULT: {
> > - struct super_block *sb = freeze_bdev(mp->m_super->s_bdev);
> > + struct super_block *sb = freeze_bdev(mp->m_super->s_bdev, 0);
>
> Using NULL here is clearer and will, I expect, avoid a sparse warning.

I checked it but I couldn't find a sparse warning in xfs_fsops.c.
Can you tell me how to use NULL?

Cheers, Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/