On Thu, 10 Jan 2013 18:10:40 +0100, David Sterba wrote: > On Thu, Jan 10, 2013 at 08:48:00PM +0800, Miao Xie wrote: >> fs_info->alloc_start was not protected strictly, it might be changed while >> we were accessing it. This patch fixes this problem by using two locks to >> protect it (fs_info->chunk_mutex and sb->s_umount). On the read side, we >> just need get one of these two locks, and on the write side, we must lock >> all of them. > > Can you please describe how this can theoretically cause any problems? > > Out of curiosity I've been running this command on a 60G ssd disk for a while > > for i in `seq 1 10000|shuf`;do > mount -o remount,alloc_start=$(($i*1024*1024)) /mnt/sdc;done > #sleep 0 / 1 / 5 > done > > together with one of my favourite stresstests (heavy writes, subvolume > activity). > > There are two direct users of fs_info->alloc_start: > > * statfs via btrfs_calc_avail_data_space > * find_free_dev_extent > > 960 search_start = max(root->fs_info->alloc_start, 1024ull * 1024); > > as remount calls sync, there is a very tiny window where an allocation could > get the old value of alloc_start just between sync and do_remount. Theoretical > and without any visible effect. ->alloc_start is a 64bits variant, on the 32bits machine, we have to load it by two instructions. Assuming -> alloc_start is 0x00010000 at the beginning, then we remount and set ->alloc_start to 0x00000100 Task0 Task1 load low 32 bits set high 32 bits load high 32 bits set low 32 bits Task1 will get 0. Thanks Miao > > My NAK for this patch. > > david > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
