* [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
@ 2018-01-12 14:21 Nikolay Borisov
2018-01-12 20:30 ` Edmund Nadolski
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-01-12 14:21 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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 1876 1859 -17
Total: Before=85966, After=85949, chg -0.02%
No functional changes.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
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(&inode->delalloc_mutex);
+ }
num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
2018-01-12 14:21 [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations Nikolay Borisov
@ 2018-01-12 20:30 ` Edmund Nadolski
2018-01-16 15:12 ` David Sterba
2018-01-30 14:34 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: Edmund Nadolski @ 2018-01-12 20:30 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
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 1876 1859 -17
> Total: Before=85966, After=85949, chg -0.02%
>
> No functional changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Edmund Nadolski <enadolski@suse.com>
> ---
> 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(&inode->delalloc_mutex);
> + }
>
> num_bytes = ALIGN(num_bytes, fs_info->sectorsize);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
2018-01-12 14:21 [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations Nikolay Borisov
2018-01-12 20:30 ` Edmund Nadolski
@ 2018-01-16 15:12 ` David Sterba
2018-01-30 14:34 ` David Sterba
2 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2018-01-16 15:12 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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 1876 1859 -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
2018-01-12 14:21 [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations Nikolay Borisov
2018-01-12 20:30 ` Edmund Nadolski
2018-01-16 15:12 ` David Sterba
@ 2018-01-30 14:34 ` David Sterba
2018-01-30 14:47 ` Nikolay Borisov
2 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2018-01-30 14:34 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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(&inode->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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
2018-01-30 14:34 ` David Sterba
@ 2018-01-30 14:47 ` Nikolay Borisov
2018-02-02 16:25 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Borisov @ 2018-01-30 14:47 UTC (permalink / raw)
To: dsterba, linux-btrfs
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(&inode->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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations
2018-01-30 14:47 ` Nikolay Borisov
@ 2018-02-02 16:25 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2018-02-02 16:25 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: dsterba, linux-btrfs
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(&inode->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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-02 16:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-12 14:21 [PATCH] btrfs: Streamline btrfs_delalloc_reserve_metadata initial operations Nikolay Borisov
2018-01-12 20:30 ` Edmund Nadolski
2018-01-16 15:12 ` David Sterba
2018-01-30 14:34 ` David Sterba
2018-01-30 14:47 ` Nikolay Borisov
2018-02-02 16:25 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).