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

[Jeff Mahoney <jeffm@suse.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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? :)

- -Jeff

- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.19 (Darwin)

iQIcBAEBAgAGBQJVeeE5AAoJEB57S2MheeWyGh8P/RvSEQxL0M7Od6YltKz3yg0H
b/pi1ZukSTNFzQQiK/tt2rgPd0XH8p139SmJ9bWiHMPD14KDN0OtsLlAIjurLUVd
Pf9jBBnaV+D8i8GVMfVBrvMqqK5xZ/6LGEFMgyqzET1WkhYZ1D7XQ5mSPKDNTqnk
YwHrbeJ7+wN0zaaqL7I2ed06Yt5e5GczpIVRXxWKsNvYsQFie4rSdnG/QUDjAReI
W4cynK6NRxQcjc0avqHRGdxFn7woyGe47tPdzr+eERE7yXr+hqrrXyzCXqWu8Nm0
f1mYX/RNRggPg6MmmjIxKsC/ySfs8p6CR6t6DejevtdQRw2uEFciqzx63y8/H3kX
L+wN/ITEOGfeCs5ndwlfL204FhZ73brY7dNIXd97yqHJhGBxadJlsoc7eI9J/fux
jNJy/wHF2/Epdfk9nBNJiRFUuL0eioYBkoE0pEdBkdoGtPAodQ+TIn+rEkxKC//n
ZovStjTJYUVuDfAi9Wpraxd6oaGmqWQU+eYxxLQBXc+ADk+lh15wmWFD9tYAw5uQ
kOwoz2PIxKCOVFBR3ixoPCy9nJsHRQX69L5xV76VqTDnGrBEDeGKHSy4Dhi8aTCN
pSJkF2STAgxxh/CHUT0FmRsIcUiLUp26pdewIjEgXQ8S6cok2/a/+HpOJRSfD0M3
bEPZLzdZREM2FhyrBcCY
=qoEa
-----END PGP SIGNATURE-----