Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations

[David Sterba]

On Tue, Jan 30, 2018 at 04:47:54PM +0200, Nikolay Borisov wrote:
> On 30.01.2018 16:34, David Sterba wrote:
> > On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote:
> >> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct 
> >> btrfs_inode *inode, u64 num_bytes)
> >> * If we have a transaction open (can happen if we call truncate_block
> >> * from truncate), then we need FLUSH_LIMIT so we don't deadlock.
> >> */
> >> +
> >>if (btrfs_is_free_space_inode(inode)) {
> >>flush = BTRFS_RESERVE_NO_FLUSH;
> >>delalloc_lock = false;
> >> -  } else if (current->journal_info) {
> >> -  flush = BTRFS_RESERVE_FLUSH_LIMIT;
> >> -  }
> >> +  } else {
> >> +  if (current->journal_info)
> >> +  flush = BTRFS_RESERVE_FLUSH_LIMIT;
> >>  
> >> -  if (flush != BTRFS_RESERVE_NO_FLUSH &&
> >> -  btrfs_transaction_in_commit(fs_info))
> >> -  schedule_timeout(1);
> >> +  if (btrfs_transaction_in_commit(fs_info))
> >> +  schedule_timeout(1);
> >>  
> >> -  if (delalloc_lock)
> >>mutex_lock(>delalloc_mutex);
> >> +  }
> > 
> > Squeezing the condition branches makes the code more readable, I have
> > only one objection and it's the mutex_lock. It IMHO looks better when
> > it's a separate branch as it pairs with the unlock:
> > 
> > if (delalloc_lock)
> > mutex_lock(...);
> > 
> > ...
> > 
> > if (delalloc_lock)
> > mutex_unlock(...);
> > 
> > In your version it's implied by the first if that checks
> > btrfs_is_free_space_inode and delalloc_lock is hidden there.
> > 
> 
> My line of thought when developing the patch was that delalloc is
> another level of indirection and. What I wanted to achieve in the end is
> to make it clear that delalloc_mutex really depends on whether we are
> reserving for the freespace inode or not.

Well, I almost overlooked the mutex on first top-down reading the code.
--
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: Streamline btrfs_delalloc_reserve_metadata initial operations

[Nikolay Borisov]

On 30.01.2018 16:34, David Sterba wrote:
> On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote:
>> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct 
>> btrfs_inode *inode, u64 num_bytes)
>>   * If we have a transaction open (can happen if we call truncate_block
>>   * from truncate), then we need FLUSH_LIMIT so we don't deadlock.
>>   */
>> +
>>  if (btrfs_is_free_space_inode(inode)) {
>>  flush = BTRFS_RESERVE_NO_FLUSH;
>>  delalloc_lock = false;
>> -} else if (current->journal_info) {
>> -flush = BTRFS_RESERVE_FLUSH_LIMIT;
>> -}
>> +} else {
>> +if (current->journal_info)
>> +flush = BTRFS_RESERVE_FLUSH_LIMIT;
>>  
>> -if (flush != BTRFS_RESERVE_NO_FLUSH &&
>> -btrfs_transaction_in_commit(fs_info))
>> -schedule_timeout(1);
>> +if (btrfs_transaction_in_commit(fs_info))
>> +schedule_timeout(1);
>>  
>> -if (delalloc_lock)
>>  mutex_lock(>delalloc_mutex);
>> +}
> 
> Squeezing the condition branches makes the code more readable, I have
> only one objection and it's the mutex_lock. It IMHO looks better when
> it's a separate branch as it pairs with the unlock:
> 
> if (delalloc_lock)
>   mutex_lock(...);
> 
> ...
> 
> if (delalloc_lock)
>   mutex_unlock(...);
> 
> In your version it's implied by the first if that checks
> btrfs_is_free_space_inode and delalloc_lock is hidden there.
> 

My line of thought when developing the patch was that delalloc is
another level of indirection and. What I wanted to achieve in the end is
to make it clear that delalloc_mutex really depends on whether we are
reserving for the freespace inode or not.

>>  
>>  num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
--
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: Streamline btrfs_delalloc_reserve_metadata initial operations

[David Sterba]

On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote:
> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct 
> btrfs_inode *inode, u64 num_bytes)
>* If we have a transaction open (can happen if we call truncate_block
>* from truncate), then we need FLUSH_LIMIT so we don't deadlock.
>*/
> +
>   if (btrfs_is_free_space_inode(inode)) {
>   flush = BTRFS_RESERVE_NO_FLUSH;
>   delalloc_lock = false;
> - } else if (current->journal_info) {
> - flush = BTRFS_RESERVE_FLUSH_LIMIT;
> - }
> + } else {
> + if (current->journal_info)
> + flush = BTRFS_RESERVE_FLUSH_LIMIT;
>  
> - if (flush != BTRFS_RESERVE_NO_FLUSH &&
> - btrfs_transaction_in_commit(fs_info))
> - schedule_timeout(1);
> + if (btrfs_transaction_in_commit(fs_info))
> + schedule_timeout(1);
>  
> - if (delalloc_lock)
>   mutex_lock(>delalloc_mutex);
> + }

Squeezing the condition branches makes the code more readable, I have
only one objection and it's the mutex_lock. It IMHO looks better when
it's a separate branch as it pairs with the unlock:

if (delalloc_lock)
mutex_lock(...);

...

if (delalloc_lock)
mutex_unlock(...);

In your version it's implied by the first if that checks
btrfs_is_free_space_inode and delalloc_lock is hidden there.

>  
>   num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
--
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: Streamline btrfs_delalloc_reserve_metadata initial operations

[David Sterba]

On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote:
> The behavior of btrfs_delalloc_reserve_metadata depends on whether
> the inode we are allocating for is the freespace inode or not. As it
> stands if we are the free node we set 'flush' and 'delalloc_lock'
> variable to certain values. Subsequently we check the values of those
> vars and act accordingly. Instead, simplify things by having 1 if
> which checks whether we are the freespace inode or not and do any
> specific operation in either branches of that if. This makes the code
> a bit easier to understand, as an added bonus it also shrinks the
> compiled size:
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17)
> Function old new   delta
> btrfs_delalloc_reserve_metadata 18761859 -17
> Total: Before=85966, After=85949, chg -0.02%

This looks too fine grained and IMHO not useful to mention. The overall
module size delta is interesting when compared between the base nad pull
request, but not for individual patches, namely if it's just 17 bytes.
--
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: Streamline btrfs_delalloc_reserve_metadata initial operations

[Edmund Nadolski]

On 01/12/2018 07:21 AM, Nikolay Borisov wrote:
> The behavior of btrfs_delalloc_reserve_metadata depends on whether
> the inode we are allocating for is the freespace inode or not. As it
> stands if we are the free node we set 'flush' and 'delalloc_lock'
> variable to certain values. Subsequently we check the values of those
> vars and act accordingly. Instead, simplify things by having 1 if
> which checks whether we are the freespace inode or not and do any
> specific operation in either branches of that if. This makes the code
> a bit easier to understand, as an added bonus it also shrinks the
> compiled size:
> 
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17)
> Function old new   delta
> btrfs_delalloc_reserve_metadata 18761859 -17
> Total: Before=85966, After=85949, chg -0.02%
> 
> No functional changes.
> 
> Signed-off-by: Nikolay Borisov 

Reviewed-by: Edmund Nadolski 

> ---
>  fs/btrfs/extent-tree.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 8d51e4bb67c1..47295804d91d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct 
> btrfs_inode *inode, u64 num_bytes)
>* If we have a transaction open (can happen if we call truncate_block
>* from truncate), then we need FLUSH_LIMIT so we don't deadlock.
>*/
> +
>   if (btrfs_is_free_space_inode(inode)) {
>   flush = BTRFS_RESERVE_NO_FLUSH;
>   delalloc_lock = false;
> - } else if (current->journal_info) {
> - flush = BTRFS_RESERVE_FLUSH_LIMIT;
> - }
> + } else {
> + if (current->journal_info)
> + flush = BTRFS_RESERVE_FLUSH_LIMIT;
>  
> - if (flush != BTRFS_RESERVE_NO_FLUSH &&
> - btrfs_transaction_in_commit(fs_info))
> - schedule_timeout(1);
> + if (btrfs_transaction_in_commit(fs_info))
> + schedule_timeout(1);
>  
> - if (delalloc_lock)
>   mutex_lock(>delalloc_mutex);
> + }
>  
>   num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
>  
> 
--
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

[PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations

[Nikolay Borisov]

The behavior of btrfs_delalloc_reserve_metadata depends on whether
the inode we are allocating for is the freespace inode or not. As it
stands if we are the free node we set 'flush' and 'delalloc_lock'
variable to certain values. Subsequently we check the values of those
vars and act accordingly. Instead, simplify things by having 1 if
which checks whether we are the freespace inode or not and do any
specific operation in either branches of that if. This makes the code
a bit easier to understand, as an added bonus it also shrinks the
compiled size:

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17 (-17)
Function old new   delta
btrfs_delalloc_reserve_metadata 18761859 -17
Total: Before=85966, After=85949, chg -0.02%

No functional changes.

Signed-off-by: Nikolay Borisov 
---
 fs/btrfs/extent-tree.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 8d51e4bb67c1..47295804d91d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode 
*inode, u64 num_bytes)
 * If we have a transaction open (can happen if we call truncate_block
 * from truncate), then we need FLUSH_LIMIT so we don't deadlock.
 */
+
if (btrfs_is_free_space_inode(inode)) {
flush = BTRFS_RESERVE_NO_FLUSH;
delalloc_lock = false;
-   } else if (current->journal_info) {
-   flush = BTRFS_RESERVE_FLUSH_LIMIT;
-   }
+   } else {
+   if (current->journal_info)
+   flush = BTRFS_RESERVE_FLUSH_LIMIT;
 
-   if (flush != BTRFS_RESERVE_NO_FLUSH &&
-   btrfs_transaction_in_commit(fs_info))
-   schedule_timeout(1);
+   if (btrfs_transaction_in_commit(fs_info))
+   schedule_timeout(1);
 
-   if (delalloc_lock)
mutex_lock(>delalloc_mutex);
+   }
 
num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
 
-- 
2.7.4

--
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