linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vivek Goyal <vgoyal@redhat.com>
To: Tejun Heo <tj@kernel.org>
Cc: axboe@kernel.dk, linux-kernel@vger.kernel.org,
	ctalbott@google.com, rni@google.com,
	containers@lists.linux-foundation.org, kay.sievers@vrfy.org
Subject: Re: [PATCH 02/10] block: fix genhd refcounting in blkio_policy_parse_and_set()
Date: Thu, 20 Oct 2011 09:41:37 -0400	[thread overview]
Message-ID: <20111020134137.GA13685@redhat.com> (raw)
In-Reply-To: <20111019235146.GS25124@google.com>

On Wed, Oct 19, 2011 at 04:51:46PM -0700, Tejun Heo wrote:
> Hello, again.
> 
> (cc'ing containers list and Kay)
> 
> The original thread is at
> 
>   http://thread.gmane.org/gmane.linux.kernel/1205150/focus=1205160
> 
> and it's about retaining blkiocg rules across device destruction.
> 
> On Wed, Oct 19, 2011 at 03:07:17PM -0700, Tejun Heo wrote:
> > On Wed, Oct 19, 2011 at 03:05:53PM -0700, Tejun Heo wrote:
> > > Hmmm.... I don't know.  If we're gonna bind rule existence to that of
> > 
> > Heh, I meant to say, "if those two are unbound,"
> > 
> > > device, wouldn't it be better to simply not check whether the device
> > > exists?  The current behavior seems pretty confusing to me.
> 
> I've been thinking about it and the more I think about it, the current
> behavior seems just wrong.  Device node # doesn't mean anything before
> a device actually appears there.  There is no way userland can know
> deterministically how the device node allocation would end up.
> 
> For example, sd allocates minors according to internal ida allocation
> which is freed on scsi_disk_release() - ie. when all *kernel*
> references go away which the userland has no way to find out until
> after new device comes up with a different devt.
> 
> For EXT_DEVT devices, this becomes even less meaingful.  There is
> absolutely no guarantee what devno would mean what.  devno which is
> currently assigned to the whole disk now can be reassigned to a
> partition.  There absolutely is no rule regarding who gets what
> numbers.  ie. This can end up with rules pointing to partitions.
> 
> Moreover, it doesn't even make the implementation simpler.  blkiocg
> currently keeps a separate list of policies so that they don't
> disappear along with blkg's.

One reason for keeping rules in blkiocg is that blkg don't get created
until and unless IO happens in that cgroup. Rules can be created much
before that.

> 
> The only way applying rules to dynamic devices can work is doing the
> proper dynamic configuration off udev and friends.

Actually it is not exactly a feature at this point of time. It was just
for the sake of simplicity that I let the rules be there even if device
has gone away and yes it is indeep a shortcoming that if a different
device shows up with old device's major and minor, then old rule will
get applied to new device.

Having said that, removal of rule upon device removal also might not
make much sense.

- Rules are tied to cgroups and not to devices as such. So until cgroup
  goes away a user might be surprised that a configured rule for a device
  suddenly disappeared.

- Though my examples are not exactly similar, but when a device goes away
  we don't try to unmount the filesystem automatically. We don't try to
  get rid of /etc/fstab entries and if somebody as put a /etc/fstab entry
  based on device name, then they might end up mounting wrong device.

So I don't feel strongly to tie rules and device life time together.
Making use of udev and friends to automatically add/remove rules as
devices show up or go will make sense though.

Thanks
Vivek

  reply	other threads:[~2011-10-20 13:42 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-19  4:26 [PATCHSET block/for-next] fix request_queue life-cycle management Tejun Heo
2011-10-19  4:26 ` [PATCH 01/10] block: make gendisk hold a reference to its queue Tejun Heo
2011-10-19  4:26 ` [PATCH 02/10] block: fix genhd refcounting in blkio_policy_parse_and_set() Tejun Heo
2011-10-19 13:26   ` Vivek Goyal
2011-10-19 16:29     ` Tejun Heo
2011-10-19 16:59       ` Vivek Goyal
2011-10-19 22:05         ` Tejun Heo
2011-10-19 22:07           ` Tejun Heo
2011-10-19 23:51             ` Tejun Heo
2011-10-20 13:41               ` Vivek Goyal [this message]
2011-10-20 16:11                 ` Tejun Heo
2011-10-20 16:16                   ` Kay Sievers
2011-10-20 17:50                     ` Vivek Goyal
2011-10-20 17:47                   ` Vivek Goyal
2011-10-19  4:26 ` [PATCH 03/10] block: move blk_throtl prototypes to block/blk.h Tejun Heo
2011-10-19 13:33   ` Vivek Goyal
2011-10-19  4:26 ` [PATCH 04/10] block: pass around REQ_* flags instead of broken down booleans during request alloc/free Tejun Heo
2011-10-19 13:44   ` Vivek Goyal
2011-10-19 16:31     ` Tejun Heo
2011-10-19  4:26 ` [PATCH 05/10] block: drop unnecessary blk_get/put_queue() in scsi_cmd_ioctl() and blk_get_tg() Tejun Heo
2011-10-19 13:52   ` Vivek Goyal
2011-10-19 16:35     ` Tejun Heo
2011-10-19  4:26 ` [PATCH 06/10] block: reorganize queue draining Tejun Heo
2011-10-19  4:26 ` [PATCH 07/10] block: reorganize throtl_get_tg() and blk_throtl_bio() Tejun Heo
2011-10-19 14:56   ` Vivek Goyal
2011-10-19 17:06     ` Tejun Heo
2011-10-19 17:19       ` Vivek Goyal
2011-10-19 17:30         ` Tejun Heo
2011-10-19 17:45           ` Vivek Goyal
2011-10-19 17:49             ` Tejun Heo
2011-10-19  4:26 ` [PATCH 08/10] block: make get_request[_wait]() fail if queue is dead Tejun Heo
2011-10-19 15:22   ` Vivek Goyal
2011-10-19  4:26 ` [PATCH 09/10] block: drop @tsk from attempt_plug_merge() and explain sync rules Tejun Heo
2011-10-19  4:26 ` [PATCH 10/10] block: fix request_queue lifetime handling by making blk_queue_cleanup() proper shutdown Tejun Heo
2011-10-19 12:43   ` Jens Axboe
2011-10-19 17:13     ` Tejun Heo
2011-10-19 18:04       ` Jens Axboe
2011-10-19 16:18   ` Vivek Goyal
2011-10-19 17:12     ` Tejun Heo
2011-10-19 17:29       ` Vivek Goyal
2011-10-19 17:33         ` Tejun Heo
2011-10-19  4:29 ` [PATCHSET block/for-next] fix request_queue life-cycle management Tejun Heo
2011-10-19 12:44 ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20111020134137.GA13685@redhat.com \
    --to=vgoyal@redhat.com \
    --cc=axboe@kernel.dk \
    --cc=containers@lists.linux-foundation.org \
    --cc=ctalbott@google.com \
    --cc=kay.sievers@vrfy.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rni@google.com \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).