Re: [PATCH 1/3] Implement generic freeze feature
From: Takashi Sato <hidden>
Date: 2008-06-27 11:33:27
Also in:
dm-devel, linux-fsdevel, linux-xfs, lkml
Hi,
quoted
+/* + * get_super_without_lock - Get super_block from block_device without lock. + * @bdev: block device struct + * + * Scan the superblock list and finds the superblock of the file system + * mounted on the block device given. This doesn't lock anyone. + * %NULL is returned if no match is found. + */This is not a terribly good comment. Which lock are we not taking? I _assume_ that it's referring to s_umount? If so, the text should describe that. It should also go to some lengths explaining why this dangerous-looking and rather nasty-looking function exists. Look at it this way: there is no way in which the reviewer of this patch (ie: me) can work out why this function exists. Hence there will be no way in which future readers of this code will be able to work out why this function exists either. This is bad. These things should be described in code comments and in the changelog (whichever is most appropriate).
Thank you for your comment. I will write comments appropriately. I was wrong. I thought we didn't need to lock s_umount because this ioctl required to open a regular file or a directory and we cannot unmount a target filesystem. So I created get_super_without_lock() used in freeze_bdev(). But, I have found that the ioctl (DM_DEV_SUSPEND_CMD in drivers/md/dm-ioctl.c) requires to open a logical volume (not a file or a directory) and calls freeze_bdev(), so we can unmount a filesystem. So I will replace get_super_without_lock with get_super to get s_umount. Cheers, Takashi