Re: [PATCH] fs: run emergency remount on dedicated workqueue

[Dave Young]

On Wed, Jun 2, 2010 at 7:46 AM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 27 May 2010 11:57:23 +0200
> Tejun Heo <tj@xxxxxxxxxx> wrote:
>
>> Commit fa4b9074cd8428958c2adf9dc0c831f46e27c193 made s_umount depend
>> on keventd;
>
> For a while I thought you had the wrong commit ID, but I worked it out!
>
> Please, always quote the patch title rather than a bare commit ID. ÂThe
> usual form is
>
> Â Âfa4b9074cd8428958c2adf9dc0c831f46e27c193 ("buffer: make
> Â Âinvalidate_bdev() drain all percpu LRU add caches:)
>
> The main reason for this is so that people can more reliably and simply
> identify the patch within a different tree. ÂI think.
>
>> however, emergency remount schedules works to keventd
>> which grabs s_umount creating a circular dependency. ÂRun emergency
>> remount on a separate workqueue to break it.
>>
>> ...
>>
>> index 69688b1..1ada607 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -575,6 +575,11 @@ int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
>> Â Â Â return 0;
>> Â}
>>
>> +/*
>> + * For emergency remount
>> + */
>> +static struct workqueue_struct *emergency_remount_wq;
>> +
>> Âstatic void do_emergency_remount(struct work_struct *work)
>> Â{
>> Â Â Â struct super_block *sb, *n;
>> @@ -605,13 +610,25 @@ void emergency_remount(void)
>> Â{
>> Â Â Â struct work_struct *work;
>>
>> + Â Â if (!emergency_remount_wq)
>> + Â Â Â Â Â Â return;
>> +
>> Â Â Â work = kmalloc(sizeof(*work), GFP_ATOMIC);
>> Â Â Â if (work) {
>> Â Â Â Â Â Â Â INIT_WORK(work, do_emergency_remount);
>> - Â Â Â Â Â Â schedule_work(work);
>> + Â Â Â Â Â Â queue_work(emergency_remount_wq, work);
>> Â Â Â }
>> Â}
>>
>> +static int __init emergency_remount_init(void)
>> +{
>> + Â Â emergency_remount_wq = create_singlethread_workqueue("emerg-remount");
>> + Â Â if (!emergency_remount_wq)
>> + Â Â Â Â Â Â pr_warn("failed to create emergency remount workqueue\n");
>> + Â Â return 0;
>> +}
>> +subsys_initcall(emergency_remount_init);
>> +
>> Â/*
>> Â * Unnamed block devices are dummy devices used by virtual
>> Â * filesystems which don't use real block-devices. Â-- jrs
>
> gaah. ÂDo we really want to add Yet Another Kernel Thread just for that
> dopey sysrq-U thing?
>
> I assume (coz you didn't tell us) that it generates a lockdep spew?
> Perhaps it'd be better to just suppress that somehow rather than this...
>
> And if we _do_ end up adding a new kernel thread for this, maybe it
> would be better to use that thread for lru_add_drain_all() rather than
> within the dopey do_emergency_remount(), so as to reduce the likelihood
> that we'll need to add even more kernel threads to solve the same
> problem elsewhere? ÂBut this would require a new kernel thread on each
> CPU, grr.
>
> Another possibility might be to change lru_add_drain_all() to use IPI
> interrupts rather than schedule_on_each_cpu(). ÂThat would greatly
> speed up lru_add_drain_all(). ÂI don't recall why we did it that way
> and I don't immediately see a reason not to. ÂA few things in core mm
> would need to be changed from spin_lock_irq() to spin_lock_irqsave().
>
> But I do have vague memories that there was a reason for it.
>
> <It's a huge PITA locating the commit which initially added
> lru_add_drain_all()>
>
> <ten minutes later>
>
> : tree 05d7615894131a368fc4943f641b11acdd2ae694
> : parent e236a166b2bc437769a9b8b5d19186a3761bde48
> : author Nick Piggin <npiggin@xxxxxxx> Thu, 19 Jan 2006 09:42:27 -0800
> : committer Linus Torvalds <torvalds@xxxxxxxxxxx> Thu, 19 Jan 2006 11:20:17 -0800
> :
> : [PATCH] mm: migration page refcounting fix
> :
> : Migration code currently does not take a reference to target page
> : properly, so between unlocking the pte and trying to take a new
> : reference to the page with isolate_lru_page, anything could happen to
> : it.
> :
> : Fix this by holding the pte lock until we get a chance to elevate the
> : refcount.
> :
> : Other small cleanups while we're here.
>
> It didn't tell us.
>
> <looks in the linux-mm archives>
>
> Nope, no rationale is provided there either.

Maybe this thread?

http://lkml.org/lkml/2008/10/23/226

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> Please read the FAQ at Âhttp://www.tux.org/lkml/
>



-- 
Regards
dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/