linux-kernel - Re: [PATCH 1/4] blkio_group key change: void * -> request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 25 Mar 2010 17:17:34 -0700
From:	Chad Talbott <ctalbott@...gle.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	jens.axboe@...cle.com, mrubin@...gle.com,
	guijianfeng@...fujitsu.com, Li Zefan <lizf@...fujitsu.com>,
	linux-kernel@...r.kernel.org, dpshah@...gle.com,
	Nauman Rafique <nauman@...gle.com>
Subject: Re: [PATCH 1/4] blkio_group key change: void * -> request_queue *

On Thu, Mar 25, 2010 at 4:25 PM, Vivek Goyal <vgoyal@...hat.com> wrote:
> human-readable disk names probably are more convenient. I got few general
> concerns.

Thanks for the quick and detailed review.

> - Can we change the sysfs interface now. In 2.6.33 kernel we released the
>  code to export blkio.time and blkio.sectors using device numbers. We
>  shall have to change those interfaces also to reflect device stats in
>  terms of device names.

I wasn't aware of those interface, but I'm all for consistency.  I'll
look into it.

> - I had kept void* as the key in blkg object so that it does not make any
>  assumption about the key. This allowed that any xyz code in kernel can
>  register with blkio code and implement some kind of IO control policy
>  and it does not have to instanciate a request queue.
>
>  But I am not sure if there will be any subsystems which will really do
>  that. I am assuming that all the device mapper and md code do
>  instanciate the request queue? (In case we implement max bw policy
>  there).

I understand your preference to keep the key generic, but I wonder if
there is *any* device that won't have a request_queue?  The penalty of
keeping it very generic might be the complexity of another auxiliary
structure to lookup gendisks or the next thing.  I wish the device
mapper folks would speak up and be involved in these discussions.

> - How do you make sure that request queue does not go away and while
>  somebody is accessing blkg->queue under rcu read lock? This can happen
>  while we call unlink code.

Here I'll admit to being very new to RCU, and I'll defer to your
worries.  More homework needed on my part.

>  blkio_unlink_group_fn().
>
>  Because we store the pointer to cfqd as key, we make sure cfqd does not
>  get deleted before one rcu grace period. (call_rcu). But this gurantees
>  only that cfqd object is around and not cfqd->queue. So this is a
>  problem with even today's code.
>
> ...
> - How do you make sure that gendisk does not go away while q->disk is
>  being accessed under rcu lock. (Already asked in other thread too.)

On locking between gendisk deletion and stats printing: I suppose that
you've already considered and discarded using the queue_lock to
protect a  gendisk pointer in request_queue?

> These are some quick thoughs. More later...

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