On Wed, 21 Jun 2017, Coly Li wrote:

On 2017/6/9 下午3:11, tang.junhui@zte.com.cn wrote:

quoted

From: Tang Junhui <redacted>

I try to execute the following command to trigger gc thread:
[root@localhost internal]# echo 1 > trigger_gc
But it does not work, I debug the code in gc_should_run(), It works only
if in invalidating or sectors_to_gc < 0. So set sectors_to_gc to -1 to
meet the condition when we trigger gc by manual command.

Hi Junhui,

Thanks for the patch, it totally makes sense IMHO. In most of use cases
I know, people writing to trigger_gc file expecting a force garbage
collection.

But assign a minus value here is quite confused for people who read the
code. Could you please also add some comments in the code to explain why
you set c->sectors_to_gc to -1 here.

I agree with Coly here.  Please do a v3 with comments explaining why -1 is 
acceptable.
 

And there might be a race that when you set c->sectors_to_gc to -1, and
call wake_up_gc(), but before code go into gc_should_run(),
c->sectors_to_gc is reset by set_gc_sectors(). I have no object if you
don't want to handle this situation, but I do suggest to explain it in
code why we can ignore such a race.

Also for v3, please address Coly's race condition concern or refute its 
existence.  Lets not introduce races if we know about them.  It should 
fail gracefully, even if it must refuse to start a gc at that moment which 
is much better than deadlocking (eg, -EINVAL the sysfs call, use a 
spinlock, or something).


--
Eric Wheeler

quoted

Signed-off-by: Tang Junhui <redacted>
---
 drivers/md/bcache/sysfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index b3ff57d..f5bb93a 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c

@@ -614,8 +614,10 @@ STORE(__bch_cache_set)
 		bch_cache_accounting_clear(&c->accounting);
 	}
 
-	if (attr == &sysfs_trigger_gc)
+	if (attr == &sysfs_trigger_gc) {
+		atomic_set(&c->sectors_to_gc, -1);
 		wake_up_gc(c);
+	}
 
 	if (attr == &sysfs_prune_cache) {
 		struct shrink_control sc;

-- 
Coly Li
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html