Re: [PATCH 1/2] btrfs: Factor out read portion of btrfs_get_blocks_direct
On Wed, Apr 11, 2018 at 10:22:58AM +0300, Nikolay Borisov wrote: > >> + free_extent_map(em); > >> + goto unlock_err; > >> + } > >> + /* > >> + * We need to unlock only the end area that we aren't using > >> + * if there is any leftover space > >> + */ > >> + free_extent_state(cached_state); > >>free_extent_map(em); > >> - goto unlock_err; > >> + return 0; > > > > Please add a separate label for that, the funcion uses the single exit > > block style (labels and one-or-two returns). > > So I think this is unnecessary because in 2/2 I factor out common code > i.e. the free_extent_map(em); return 0; outside of the 'if' branch and > so this return disappears. The 3rd hunk in 2/2 begins with: > > @@ -7780,106 +7882,8 @@ static int btrfs_get_blocks_direct(struct inode > *inode, sector_t iblock, >* if there is any leftover space >*/ > free_extent_state(cached_state); > - free_extent_map(em); > - return 0; > - } Ok. -- 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 1/2] btrfs: Factor out read portion of btrfs_get_blocks_direct
On 10.04.2018 18:49, David Sterba wrote:
> On Thu, Feb 22, 2018 at 06:12:13PM +0200, Nikolay Borisov wrote:
>> Currently this function handles both the READ and WRITE dio cases. This
>> is facilitated by a bunch of 'if' statements, a goto short-circuit
>> statement and a very perverse aliasing of "!created"(READ) case
>> by setting lockstart = lockend and checking for lockstart < lockend for
>> detecting the write. Let's simplify this mess by extracting the
>> READ-only code into a separate __btrfs_get_block_direct_read function.
>> This is only the first step, the next one will be to factor out the
>> write side as well. The end goal will be to have the common locking/
>> unlocking code in btrfs_get_blocks_direct and then it will call either
>> the read|write subvariants. No functional changes.
>
> Reviewed-by: David Sterba
>
> uh what a convoluted code to untangle. A few style notes below.
>>
>> Signed-off-by: Nikolay Borisov
>> ---
>> fs/btrfs/inode.c | 49 -
>> 1 file changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 491a7397f6fa..fd99347d0c91 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -7677,6 +7677,27 @@ static struct extent_map *create_io_em(struct inode
>> *inode, u64 start, u64 len,
>> return em;
>> }
>>
>> +
>> +static int __btrfs_get_blocks_direct_read(struct extent_map *em,
>
> Please drop the "__" the function is static and there's no
> underscore-less version.
>
>> + struct buffer_head *bh_result,
>> + struct inode *inode,
>> + u64 start, u64 len)
>> +{
>> +if (em->block_start == EXTENT_MAP_HOLE ||
>> +test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
>> +return -ENOENT;
>> +
>> +len = min(len, em->len - (start - em->start));
>> +
>> +bh_result->b_blocknr = (em->block_start + (start - em->start)) >>
>> +inode->i_blkbits;
>> +bh_result->b_size = len;
>> +bh_result->b_bdev = em->bdev;
>> +set_buffer_mapped(bh_result);
>> +
>> +return 0;
>> +}
>> +
>> static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>> struct buffer_head *bh_result, int create)
>> {
>> @@ -7745,11 +7766,22 @@ static int btrfs_get_blocks_direct(struct inode
>> *inode, sector_t iblock,
>> goto unlock_err;
>> }
>>
>> -/* Just a good old fashioned hole, return */
>> -if (!create && (em->block_start == EXTENT_MAP_HOLE ||
>> -test_bit(EXTENT_FLAG_PREALLOC, &em->flags))) {
>> +if (!create) {
>> +ret = __btrfs_get_blocks_direct_read(em, bh_result, inode,
>> + start, len);
>> +/* Can be negative only if we read from a hole */
>> +if (ret < 0) {
>> +ret = 0;
>> +free_extent_map(em);
>> +goto unlock_err;
>> +}
>> +/*
>> + * We need to unlock only the end area that we aren't using
>> + * if there is any leftover space
>> + */
>> +free_extent_state(cached_state);
>> free_extent_map(em);
>> -goto unlock_err;
>> +return 0;
>
> Please add a separate label for that, the funcion uses the single exit
> block style (labels and one-or-two returns).
So I think this is unnecessary because in 2/2 I factor out common code
i.e. the free_extent_map(em); return 0; outside of the 'if' branch and
so this return disappears. The 3rd hunk in 2/2 begins with:
@@ -7780,106 +7882,8 @@ static int btrfs_get_blocks_direct(struct inode
*inode, sector_t iblock,
* if there is any leftover space
*/
free_extent_state(cached_state);
- free_extent_map(em);
- return 0;
- }
>
>> }
>>
>> /*
>> @@ -7761,12 +7793,6 @@ static int btrfs_get_blocks_direct(struct inode
>> *inode, sector_t iblock,
>> * just use the extent.
>> *
>> */
>> -if (!create) {
>> -len = min(len, em->len - (start - em->start));
>> -lockstart = start + len;
>> -goto unlock;
>> -}
>> -
>> if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) ||
>> ((BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) &&
>> em->block_start != EXTENT_MAP_HOLE)) {
>> @@ -7853,10 +7879,7 @@ static int btrfs_get_blocks_direct(struct inode
>> *inode, sector_t iblock,
>> clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart,
>> lockend, unlock_bits, 1, 0,
>> &cached_state);
>> -} else {
>> -free_extent_state(cached_state);
>> }
>> -
>> free_extent_map(em);
>>
>> return 0;
--
To unsubscrib
Re: [PATCH 1/2] btrfs: Factor out read portion of btrfs_get_blocks_direct
On Thu, Feb 22, 2018 at 06:12:13PM +0200, Nikolay Borisov wrote:
> Currently this function handles both the READ and WRITE dio cases. This
> is facilitated by a bunch of 'if' statements, a goto short-circuit
> statement and a very perverse aliasing of "!created"(READ) case
> by setting lockstart = lockend and checking for lockstart < lockend for
> detecting the write. Let's simplify this mess by extracting the
> READ-only code into a separate __btrfs_get_block_direct_read function.
> This is only the first step, the next one will be to factor out the
> write side as well. The end goal will be to have the common locking/
> unlocking code in btrfs_get_blocks_direct and then it will call either
> the read|write subvariants. No functional changes.
Reviewed-by: David Sterba
uh what a convoluted code to untangle. A few style notes below.
>
> Signed-off-by: Nikolay Borisov
> ---
> fs/btrfs/inode.c | 49 -
> 1 file changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 491a7397f6fa..fd99347d0c91 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7677,6 +7677,27 @@ static struct extent_map *create_io_em(struct inode
> *inode, u64 start, u64 len,
> return em;
> }
>
> +
> +static int __btrfs_get_blocks_direct_read(struct extent_map *em,
Please drop the "__" the function is static and there's no
underscore-less version.
> + struct buffer_head *bh_result,
> + struct inode *inode,
> + u64 start, u64 len)
> +{
> + if (em->block_start == EXTENT_MAP_HOLE ||
> + test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
> + return -ENOENT;
> +
> + len = min(len, em->len - (start - em->start));
> +
> + bh_result->b_blocknr = (em->block_start + (start - em->start)) >>
> + inode->i_blkbits;
> + bh_result->b_size = len;
> + bh_result->b_bdev = em->bdev;
> + set_buffer_mapped(bh_result);
> +
> + return 0;
> +}
> +
> static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> struct buffer_head *bh_result, int create)
> {
> @@ -7745,11 +7766,22 @@ static int btrfs_get_blocks_direct(struct inode
> *inode, sector_t iblock,
> goto unlock_err;
> }
>
> - /* Just a good old fashioned hole, return */
> - if (!create && (em->block_start == EXTENT_MAP_HOLE ||
> - test_bit(EXTENT_FLAG_PREALLOC, &em->flags))) {
> + if (!create) {
> + ret = __btrfs_get_blocks_direct_read(em, bh_result, inode,
> +start, len);
> + /* Can be negative only if we read from a hole */
> + if (ret < 0) {
> + ret = 0;
> + free_extent_map(em);
> + goto unlock_err;
> + }
> + /*
> + * We need to unlock only the end area that we aren't using
> + * if there is any leftover space
> + */
> + free_extent_state(cached_state);
> free_extent_map(em);
> - goto unlock_err;
> + return 0;
Please add a separate label for that, the funcion uses the single exit
block style (labels and one-or-two returns).
> }
>
> /*
> @@ -7761,12 +7793,6 @@ static int btrfs_get_blocks_direct(struct inode
> *inode, sector_t iblock,
>* just use the extent.
>*
>*/
> - if (!create) {
> - len = min(len, em->len - (start - em->start));
> - lockstart = start + len;
> - goto unlock;
> - }
> -
> if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) ||
> ((BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) &&
>em->block_start != EXTENT_MAP_HOLE)) {
> @@ -7853,10 +7879,7 @@ static int btrfs_get_blocks_direct(struct inode
> *inode, sector_t iblock,
> clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart,
>lockend, unlock_bits, 1, 0,
>&cached_state);
> - } else {
> - free_extent_state(cached_state);
> }
> -
> free_extent_map(em);
>
> return 0;
--
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
