Re: [PATCH 1/2] btrfs: cleaner_kthread() doesn't need explicit freeze
On Tue, 15 Mar 2016, Jiri Kosina wrote:
> cleaner_kthread() is not marked freezable, and therefore calling
> try_to_freeze() in its context is a pointless no-op.
>
> In addition to that, as has been clearly demonstrated by 80ad623edd2d
> ("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()"), it's perfectly
> valid / legal for cleaner_kthread() to stay scheduled out in an arbitrary
> place during suspend (in that particular example that was waiting for
> reading of extent pages), so there is no need to leave any traces of
> freezer in this kthread.
Given some questions I've received offline, let me clarify a little bit
more here.
Currently, the try_to_freeze() call is completely useless here, because it
will never actually try to freeze the kthread (as it's PF_NOFREEZE).
When attempted to make the kthread properly freezable, it turned out (see
e.g. 80ad623edd2d) that it's actually sleeping in various places during
suspend for long periods of time (my guess would be that it doesn't really
matter whether the cleaning happens before or after suspend, but this'd be
something I'd like to have clarified from btrfs folks).
So in a nutshell, this patch (a) doesn't make things worse, as it's an
equivalent code transformation (b) brings more sanity to how the kthread
freezing API is used throughout the kernel.
It might very well be that the code was broken before; but it's not more
broken after this patch, and the API usage is sane.
The ultimate goal is first to bring some sanity into how the freezer API
is used throughout the kernel, and then eventually get rid of it
completely in favor of fs freezing (currently it's not even possible to
analyze all the uses in the kernel, as there are way too many and most of
them are totally broken).
--
Jiri Kosina
SUSE Labs
--
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 2/2] btrfs: transaction_kthread() is not freezable
transaction_kthread() is calling try_to_freeze(), but that's just an
expeinsive no-op given the fact that the thread is not marked freezable.
After removing this, disk-io.c is now independent on freezer API.
Signed-off-by: Jiri Kosina <jkos...@suse.cz>
---
fs/btrfs/disk-io.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d8d68af..4c7361a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -25,7 +25,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -1920,14 +1919,12 @@ sleep:
if (unlikely(test_bit(BTRFS_FS_STATE_ERROR,
>fs_info->fs_state)))
btrfs_cleanup_transaction(root);
- if (!try_to_freeze()) {
- set_current_state(TASK_INTERRUPTIBLE);
- if (!kthread_should_stop() &&
- (!btrfs_transaction_blocked(root->fs_info) ||
-cannot_commit))
- schedule_timeout(delay);
- __set_current_state(TASK_RUNNING);
- }
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (!kthread_should_stop() &&
+ (!btrfs_transaction_blocked(root->fs_info) ||
+cannot_commit))
+ schedule_timeout(delay);
+ __set_current_state(TASK_RUNNING);
} while (!kthread_should_stop());
return 0;
}
--
Jiri Kosina
SUSE Labs
--
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 1/2] btrfs: cleaner_kthread() doesn't need explicit freeze
cleaner_kthread() is not marked freezable, and therefore calling
try_to_freeze() in its context is a pointless no-op.
In addition to that, as has been clearly demonstrated by 80ad623edd2d
("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()"), it's perfectly
valid / legal for cleaner_kthread() to stay scheduled out in an arbitrary
place during suspend (in that particular example that was waiting for
reading of extent pages), so there is no need to leave any traces of
freezer in this kthread.
Fixes: 80ad623edd2d ("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()")
Fixes: 696249132158 ("btrfs: clear PF_NOFREEZE in cleaner_kthread()")
Signed-off-by: Jiri Kosina <jkos...@suse.cz>
---
fs/btrfs/disk-io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4545e2e..d8d68af 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1830,7 +1830,7 @@ static int cleaner_kthread(void *arg)
*/
btrfs_delete_unused_bgs(root->fs_info);
sleep:
- if (!try_to_freeze() && !again) {
+ if (!again) {
set_current_state(TASK_INTERRUPTIBLE);
if (!kthread_should_stop())
schedule();
--
Jiri Kosina
SUSE Labs
--
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: clear PF_NOFREEZE in cleaner_kthread()
From: Jiri Kosina <jkos...@suse.cz>
cleaner_kthread() kthread calls try_to_freeze() at the beginning of every
cleanup attempt. This operation can't ever succeed though, as the kthread
hasn't marked itself as freezable.
Before (hopefully eventually) kthread freezing gets converted to fileystem
freezing, we'd rather mark cleaner_kthread() freezable (as my
understanding is that it can generate filesystem I/O during suspend).
Signed-off-by: Jiri Kosina <jkos...@suse.cz>
---
fs/btrfs/disk-io.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 295795a..173970d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1759,6 +1759,7 @@ static int cleaner_kthread(void *arg)
int again;
struct btrfs_trans_handle *trans;
+ set_freezable();
do {
again = 0;
--
Jiri Kosina
SUSE Labs
--
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: reada: Remove unused function
On Mon, 5 Jan 2015, David Sterba wrote: Remove the function btrfs_reada_detach() that is not used anywhere. This was partially found by using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se No please, this function is part of public readahead API and similar patch has been NACKed several times. BTW how is this any kind of API for anybody, given it's not exported to modules? -- Jiri Kosina SUSE Labs -- 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 3/4] Btrfs: remove unnecessary cur_trans set before goto loop in join_transaction
On Mon, 24 Sep 2012, Wang Sheng-Hui wrote:
In the big loop, cur_trans will be set fs_info-running_transaction
before it's used. And after kmem_cache_free it and goto loop, it will
be setup again. No need to setup it immediately after freed.
Signed-off-by: Wang Sheng-Hui shh...@gmail.com
---
fs/btrfs/transaction.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 469a8b6..675d813 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -98,7 +98,6 @@ loop:
* to redo the trans_no_join checks above
*/
kmem_cache_free(btrfs_transaction_cachep, cur_trans);
- cur_trans = fs_info-running_transaction;
goto loop;
} else if (fs_info-fs_state BTRFS_SUPER_FLAG_ERROR) {
spin_unlock(fs_info-trans_lock);
This is Obviously Correct(TM) :-) and doesn't seem to have been picked up
by btrfs maintainers. I am picking it up now.
--
Jiri Kosina
SUSE Labs
--
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: fix typo in cow_file_range_async and async_cow_submit
From: Liu Bo liubo2...@cn.fujitsu.com It should be 10 * 1024 * 1024. Signed-off-by: Liu Bo liubo2...@cn.fujitsu.com Signed-off-by: Jiri Kosina jkos...@suse.cz --- fs/btrfs/inode.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 3bb6b25..a0e2c1b 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -1008,7 +1008,7 @@ static noinline void async_cow_submit(struct btrfs_work *work) atomic_sub(nr_pages, root-fs_info-async_delalloc_pages); if (atomic_read(root-fs_info-async_delalloc_pages) - 5 * 1042 * 1024 + 5 * 1024 * 1024 waitqueue_active(root-fs_info-async_submit_wait)) wake_up(root-fs_info-async_submit_wait); @@ -1031,7 +1031,7 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page, struct btrfs_root *root = BTRFS_I(inode)-root; unsigned long nr_pages; u64 cur_end; - int limit = 10 * 1024 * 1042; + int limit = 10 * 1024 * 1024; clear_extent_bit(BTRFS_I(inode)-io_tree, start, end, EXTENT_LOCKED, 1, 0, NULL, GFP_NOFS); -- Jiri Kosina SUSE Labs -- 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] Make an error msg look nicer by inserting a space between number and word.
On Mon, 1 Jun 2009, Hu Tao wrote: Signed-off-by: Hu Tao hu.t...@gmail.com --- fs/btrfs/extent-tree.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 3e2c7c7..8bb0550 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -1843,7 +1843,7 @@ again: printk(KERN_ERR no space left, need %llu, %llu delalloc bytes , %llu bytes_used, %llu bytes_reserved, -%llu bytes_pinned, %llu bytes_readonly, %llu may use +%llu bytes_pinned, %llu bytes_readonly, %llu may use %llu total\n, (unsigned long long)bytes, (unsigned long long)data_sinfo-bytes_delalloc, (unsigned long long)data_sinfo-bytes_used, Applied to trivial tree. If it has been picked up by subsystem tree, please let me know and I will drop it. Thanks, -- Jiri Kosina -- 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] Spelling fix in btrfs code comment
On Sat, 9 May 2009, Sankar P wrote: Fix a trivial spelling error in a comment Signed-off-by: Sankar P sankar.curios...@gmail.com --- diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index e496644..3e2c7c7 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -312,7 +312,7 @@ btrfs_lookup_first_block_group(struct btrfs_fs_info *info, u64 bytenr) } /* - * return the block group that contains teh given bytenr + * return the block group that contains the given bytenr */ struct btrfs_block_group_cache *btrfs_lookup_block_group( struct btrfs_fs_info *info, Appplied to trivial tree. If it has been already applied to btrfs pileup, please let me know so that I could drop it. Thanks, -- Jiri Kosina SUSE Labs -- 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 -v7][RFC]: mutex: implement adaptive spinning
On Thu, 8 Jan 2009, Peter Zijlstra wrote: Well, at least we do unless you enable that broken paravirt support. I'm not at all clear on why CONFIG_PARAVIRT wants to use inferior locks, but I don't much care. Because the virtual cpu that has the ticket might not get scheduled for a while, even though another vcpu with a spinner is scheduled. The whole (para)virt is a nightmare in that respect. Hmm, are we in fact really using byte locks in CONFIG_PARAVIRT situation? Where are we actually setting pv_lock_ops.spin_lock pointer to point to __byte_spin_lock? Such initialization seems to happen only in paravirt_use_bytelocks() function, but my blind eyes prevent me from finding a callsite from which this function would eventually get called. -- Jiri Kosina SUSE Labs -- 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
