Messages in this thread | | | From | "Takashi Sato" <> | Subject | Re: [PATCH 1/3] Implement generic freeze feature | Date | Fri, 27 Jun 2008 20:33:27 +0900 |
| |
Hi,
>> +/* >> + * 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
| |