Re: [PATCH 1/4] btrfs: skip superblocks during discard

[Chris Mason <clm@fb.com>]

On 06/11/2015 03:27 PM, Jeff Mahoney wrote:
> On 6/11/15 3:24 PM, Chris Mason wrote:
>> On 06/11/2015 03:15 PM, Jeff Mahoney wrote:
>>> On 6/11/15 2:44 PM, Filipe David Manana wrote:
>>>> On Thu, Jun 11, 2015 at 7:17 PM, Jeff Mahoney <jeffm@suse.com> 
>>>> wrote: On 6/11/15 12:47 PM, Filipe David Manana wrote:
>>>>>>> On Thu, Jun 11, 2015 at 4:20 PM,  <jeffm@suse.com>
>>>>>>> wrote:
>>>>>>>> From: Jeff Mahoney <jeffm@suse.com>
>>>>>>>>
>>>>>>>> Btrfs doesn't track superblocks with extent records so 
>>>>>>>> there is nothing persistent on-disk to indicate that
>>>>>>>> those blocks are in use.  We track the superblocks in
>>>>>>>> memory to ensure they don't get used by removing them
>>>>>>>> from the free space cache when we load a block group
>>>>>>>> from disk.  Prior to 47ab2a6c6a (Btrfs: remove empty
>>>>>>>> block groups automatically), that was fine since the
>>>>>>>> block group would never be reclaimed so the superblock
>>>>>>>> was always safe. Once we started removing the empty
>>>>>>>> block groups, we were protected by the fact that
>>>>>>>> discards weren't being properly issued for unused space
>>>>>>>> either via FITRIM or -odiscard. The block groups were
>>>>>>>> still being released, but the blocks remained on disk.
>>>>>>>>
>>>>>>>> In order to properly discard unused block groups, we
>>>>>>>> need to filter out the superblocks from the discard
>>>>>>>> range. Superblocks are located at fixed locations on
>>>>>>>> each device, so it makes sense to filter them out in 
>>>>>>>> btrfs_issue_discard, which is used by both -odiscard
>>>>>>>> and FITRIM.
>>>>>>>>
>>>>>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com> --- 
>>>>>>>> fs/btrfs/extent-tree.c | 50 
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++------ 1
>>>>>>>> file changed, 44 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/extent-tree.c 
>>>>>>>> b/fs/btrfs/extent-tree.c index 0ec3acd..75d0226 100644
>>>>>>>> --- a/fs/btrfs/extent-tree.c +++
>>>>>>>> b/fs/btrfs/extent-tree.c @@ -1884,10 +1884,47 @@ static
>>>>>>>> int remove_extent_backref(struct btrfs_trans_handle
>>>>>>>> *trans, return ret; }
>>>>>>>>
>>>>>>>> -static int btrfs_issue_discard(struct block_device
>>>>>>>> *bdev, - u64 start, u64 len) +#define in_range(b,
>>>>>>>> first, len) ((b)
>>>>>>>>> = (first) && (b) < (first) + (len))
>>>>>>>
>>>>>>> Hi Jeff,
>>>>>>>
>>>>>>> So this will work if every caller behaves well and passes
>>>>>>> a region whose start and end offsets are a multiple of
>>>>>>> the sector size (4096) which currently matches the
>>>>>>> superblock size.
>>>>>>>
>>>>>>> However, I think it would be safer to check for the case 
>>>>>>> where the start offset of a superblock mirror is <
>>>>>>> (first) and (sb_offset + sb_len) > (first).  Just to deal
>>>>>>> with cases where for example the 2nd half of the sb
>>>>>>> starts at offset (first).
>>>>>>>
>>>>>>> I guess this sectorsize becoming less than 4096 will
>>>>>>> happen sooner or later with the subpage sectorsize patch
>>>>>>> set, so it wouldn't hurt to make it more bullet proof
>>>>>>> already.
>>>
>>>> Is that something anyone intends to support?  While I suppose
>>>> the subpage sector patch /could/ be used to allow file systems
>>>> with a node size under 4k, the intention is the other way
>>>> around -- systems that have higher order page sizes currently
>>>> don't work with btrfs file system created on systems with
>>>> smaller order page sizes like x86.
> 
>> The best use of smaller node sizes is just to test the subpagesize 
>> patches on more common hardware.  I wouldn't expect anyone to use a
>> 1K node size in production.
> 
> Any chance we can enforce that?  Like with a compile-time option? :)

We can make mkfs.btrfs advise strongly against it ;)

But, since I wasn't horribly clear, I'd love one extra if statement in
the discard function.  Silently eating bytes is horribly hard to track down.

-chris