Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Josef Bacik wrote: > We used to store the checksums of the space cache directly in the space cache, > however that doesn't work out too well if we have more space than we can fit > the > checksums into the first page. So instead use the normal checksumming > infrastructure. There were problems with doing this originally but those > problems don't exist now so this works out fine. Thanks, > This looks great, so I'll drop my patch that extends the original code to allow more than 1 crc page. one comment below: ... > @@ -879,11 +802,7 @@ int __btrfs_write_out_cache(struct btrfs_root *root, > struct inode *inode, > > ret = 1; > > -out_free: > - kfree(checksums); > - kfree(pages); > - leak memory by removing kfree(pages). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400: > Josef Bacik wrote: > > We used to store the checksums of the space cache directly in the space > > cache, > > however that doesn't work out too well if we have more space than we can > > fit the > > checksums into the first page. So instead use the normal checksumming > > infrastructure. There were problems with doing this originally but those > > problems don't exist now so this works out fine. Thanks, > > > > This looks great, so I'll drop my patch that extends the original code to > allow more than 1 crc page. I do like them a lot, but what happens when a special case crc kernel mounts a free space cache created by this patch? -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Chris Mason wrote:
> Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400:
>> Josef Bacik wrote:
>>> We used to store the checksums of the space cache directly in the space
>>> cache,
>>> however that doesn't work out too well if we have more space than we can
>>> fit the
>>> checksums into the first page. So instead use the normal checksumming
>>> infrastructure. There were problems with doing this originally but those
>>> problems don't exist now so this works out fine. Thanks,
>>>
>>
>> This looks great, so I'll drop my patch that extends the original code to
>> allow more than 1 crc page.
>
> I do like them a lot, but what happens when a special case crc kernel
> mounts a free space cache created by this patch?
>
So we need a check, like the one in load_free_space_cache():
@@ -2650,6 +2650,11 @@ int load_free_ino_cache(struct btrfs_fs_info *fs_info, st
if (IS_ERR(inode))
goto out;
+ if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) {
+ printk(KERN_INFO "Old style space inode found, converting.\n");
+ BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODATASUM;
+ }
+
if (root_gen != BTRFS_I(inode)->generation)
goto out_put;
then we'll not trying to load the cache from disk but reconstruct the
cache by searching the fs tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Li Zefan wrote: > Chris Mason wrote: >> Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400: >>> Josef Bacik wrote: We used to store the checksums of the space cache directly in the space cache, however that doesn't work out too well if we have more space than we can fit the checksums into the first page. So instead use the normal checksumming infrastructure. There were problems with doing this originally but those problems don't exist now so this works out fine. Thanks, >>> >>> This looks great, so I'll drop my patch that extends the original code to >>> allow more than 1 crc page. >> >> I do like them a lot, but what happens when a special case crc kernel >> mounts a free space cache created by this patch? >> > > So we need a check, like the one in load_free_space_cache(): > oh I misunderstood your question.. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Chris Mason wrote: > Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400: >> Josef Bacik wrote: >>> We used to store the checksums of the space cache directly in the space >>> cache, >>> however that doesn't work out too well if we have more space than we can >>> fit the >>> checksums into the first page. So instead use the normal checksumming >>> infrastructure. There were problems with doing this originally but those >>> problems don't exist now so this works out fine. Thanks, >>> >> >> This looks great, so I'll drop my patch that extends the original code to >> allow more than 1 crc page. > > I do like them a lot, but what happens when a special case crc kernel > mounts a free space cache created by this patch? > The generation number will be checked, and then we'll get this warning: "btrfs: space cache generation xx does not match inode yy" If xx happens to be equal to yy, crcs will be checked and this warning will pop up: "btrfs: crc mismatch for page zz" but it won't crash (not tested). -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
On 06/12/2011 09:52 PM, Li Zefan wrote: > Josef Bacik wrote: >> We used to store the checksums of the space cache directly in the space >> cache, >> however that doesn't work out too well if we have more space than we can fit >> the >> checksums into the first page. So instead use the normal checksumming >> infrastructure. There were problems with doing this originally but those >> problems don't exist now so this works out fine. Thanks, >> > > This looks great, so I'll drop my patch that extends the original code to > allow more than 1 crc page. > > one comment below: > > ... >> @@ -879,11 +802,7 @@ int __btrfs_write_out_cache(struct btrfs_root *root, >> struct inode *inode, >> >> ret = 1; >> >> -out_free: >> -kfree(checksums); >> -kfree(pages); >> - > > leak memory by removing kfree(pages). Ah right good catch, I'll fix that up, thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
Excerpts from Li Zefan's message of 2011-06-12 22:20:43 -0400: > Chris Mason wrote: > > Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400: > >> Josef Bacik wrote: > >>> We used to store the checksums of the space cache directly in the space > >>> cache, > >>> however that doesn't work out too well if we have more space than we can > >>> fit the > >>> checksums into the first page. So instead use the normal checksumming > >>> infrastructure. There were problems with doing this originally but those > >>> problems don't exist now so this works out fine. Thanks, > >>> > >> > >> This looks great, so I'll drop my patch that extends the original code to > >> allow more than 1 crc page. > > > > I do like them a lot, but what happens when a special case crc kernel > > mounts a free space cache created by this patch? > > > > The generation number will be checked, and then we'll get this warning: > > "btrfs: space cache generation xx does not match inode yy" > > If xx happens to be equal to yy, crcs will be checked and this warning > will pop up: > > "btrfs: crc mismatch for page zz" > > but it won't crash (not tested). Right, but that's a lot of trust to put into our small crcs. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: use the normal checksumming infrastructure for free space cache
On 06/13/2011 10:11 AM, Chris Mason wrote: > Excerpts from Li Zefan's message of 2011-06-12 22:20:43 -0400: >> Chris Mason wrote: >>> Excerpts from Li Zefan's message of 2011-06-12 21:52:32 -0400: Josef Bacik wrote: > We used to store the checksums of the space cache directly in the space > cache, > however that doesn't work out too well if we have more space than we can > fit the > checksums into the first page. So instead use the normal checksumming > infrastructure. There were problems with doing this originally but those > problems don't exist now so this works out fine. Thanks, > This looks great, so I'll drop my patch that extends the original code to allow more than 1 crc page. >>> >>> I do like them a lot, but what happens when a special case crc kernel >>> mounts a free space cache created by this patch? >>> >> >> The generation number will be checked, and then we'll get this warning: >> >> "btrfs: space cache generation xx does not match inode yy" >> >> If xx happens to be equal to yy, crcs will be checked and this warning >> will pop up: >> >> "btrfs: crc mismatch for page zz" >> >> but it won't crash (not tested). > > Right, but that's a lot of trust to put into our small crcs. Yeah we're trusting that the crc's or the generation won't match, which really should be the case always. Unfortunately there is no real way to force the old kernels to flush the space cache with the new format, we just have to assume that it will get the wrong checksum, which really it should. Course now that I've said this some user will hit that one in a hundred trillion chance that the entries will end up having valid crc's for the pages. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
