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

Takashi Sato
2008-08-29T09:36:31Z

Hi,

Andrew Morton wrote:
>> --- linux-2.6.27-rc2.org/include/linux/fs.h 2008-08-06 13:49:54.000000000 +0900
>> +++ linux-2.6.27-rc2-freeze/include/linux/fs.h 2008-08-07 08:59:54.000000000 +0900
>> @@ -226,6 +226,8 @@ extern int dir_notify_enable;
>> #define BMAP_IOCTL 1 /* obsolete - kept for compatibility */
>> #define FIBMAP _IO(0x00,1) /* bmap access */
>> #define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */
>> +#define FIFREEZE _IOWR('X', 119, int) /* Freeze */
>> +#define FITHAW _IOWR('X', 120, int) /* Thaw */
>
>FIFREEZE is 119, but a few lines above we have
>
>#define BLKDISCARD _IO(0x12,119)
>
>Should we be using 120 and 121 here?

As Andreas said, we need to use 'X' to keep compatibility with
XFS's freeze ioctl.

>
>> #define FS_IOC_GETFLAGS _IOR('f', 1, long)
>> #define FS_IOC_SETFLAGS _IOW('f', 2, long)
>> @@ -574,6 +576,10 @@ struct block_device {
>> * care to not mess up bd_private for that case.
>> */
>> unsigned long bd_private;
>> + /* The counter of freeze processes */
>> + int bd_freeze_count;
>> + /* Semaphore for freeze */
>> + struct semaphore bd_freeze_sem;
>
>"freeze" is not an adequate description of what this protects. I think
>it's only the modification and testing of bd_freeze_count, isn't it?
>
>If so, all this could be done more neatly by removing the lock,
>switching to atomic_t and using our (rich) atomic_t operations.
>
>otoh, perhaps it protects more than this, in which case the lock
>can/should be switched to a `struct mutex'?

bd_freeze_sem protects the following two sequences.
1. freeze_bdev()
- Test of bd_freeze_count
- Increment of bd_freeze_count
- s_op->write_super_lockfs
- Set unfreeze timer

2. thaw_bdev()
- Test of bd_freeze_count
- Decrement of bd_freeze_count
- s_op->unlockfs
- Unset unfreeze timer
Because the journal sync in ext3's write_super_lockfs might
need a long time, we should use the mutex (not atomic_t).
If bd_freeze_sem protects only the modification and
testing of bd_freeze_count, freeze_bdev() and thaw_bdev() will
run simultaneously and unexpected problem will occur.
(For example, after we run the freeze ioctl with timeout period,
the filesystem is frozen, but the unfreeze timer isn't set.)

Cheers, Takashi