lkml.org 
[lkml]   [2008]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 6/8] bio-cgroup: The body of bio-cgroup
    From
    Hi,

    As you pointed out, cgroup iocontext stuff isn't well designed yet
    since the current implementation of dm-iband doesn't need it.
    I'd like to leave the iocontext stuff to you I/O scheduler guys
    if you want to implement the bio-cgroup infrastructure to handle iocotexts
    as the I/O schedulers expect.

    > Hi,
    >
    > Ryo Tsuruta wrote:
    > >
    > > +void init_io_context(struct io_context *ioc)
    > > +{
    > > + atomic_set(&ioc->refcount, 1);
    > > + atomic_set(&ioc->nr_tasks, 1);
    > > + spin_lock_init(&ioc->lock);
    > > + ioc->ioprio_changed = 0;
    > > + ioc->ioprio = 0;
    > > + ioc->last_waited = jiffies; /* doesn't matter... */
    > > + ioc->nr_batch_requests = 0; /* because this is 0 */
    > > + ioc->aic = NULL;
    > > + INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
    > > + INIT_HLIST_HEAD(&ioc->cic_list);
    > > + ioc->ioc_data = NULL;
    > > +}
    > > +
    > > struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
    > > {
    > > struct io_context *ret;
    > >
    > > ret = kmem_cache_alloc_node(iocontext_cachep, gfp_flags, node);
    > > - if (ret) {
    > > - atomic_set(&ret->refcount, 1);
    > > - atomic_set(&ret->nr_tasks, 1);
    > > - spin_lock_init(&ret->lock);
    > > - ret->ioprio_changed = 0;
    > > - ret->ioprio = 0;
    > > - ret->last_waited = jiffies; /* doesn't matter... */
    > > - ret->nr_batch_requests = 0; /* because this is 0 */
    > > - ret->aic = NULL;
    > > - INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC | __GFP_HIGH);
    > > - INIT_HLIST_HEAD(&ret->cic_list);
    > > - ret->ioc_data = NULL;
    > > - }
    > > + if (ret)
    > > + init_io_context(ret);
    > >
    > > return ret;
    > > }
    >
    >
    > > +
    > > +/* Create a new bio-cgroup. */
    > > +static struct cgroup_subsys_state *
    > > +bio_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cgrp)
    > > +{
    > > + struct bio_cgroup *biog;
    > > + struct io_context *ioc;
    > > + int ret;
    > > +
    > > + if (!cgrp->parent) {
    > > + biog = &default_bio_cgroup;
    > > + init_io_context(biog->io_context);
    > > + /* Increment the referrence count not to be released ever. */
    > > + atomic_inc(&biog->io_context->refcount);
    > > + idr_init(&bio_cgroup_id);
    > > + return &biog->css;
    > > + }
    > > +
    > > + biog = kzalloc(sizeof(*biog), GFP_KERNEL);
    > > + ioc = alloc_io_context(GFP_KERNEL, -1);
    > > + if (!ioc || !biog) {
    > > + ret = -ENOMEM;
    > > + goto out_err;
    > > + }
    > > + biog->io_context = ioc;
    >
    > If I understand correctly io_contexts allocated by alloc_io_context() should
    > be owned by some tasks. In this case, the newly created io_context has no
    > owner though biog->io_context->nr_tasks == 1. With the cfq scheduler this may
    > cause some problems especially when cic_free_func() is called because
    > cfq_exit_io_context() would never be called. Am I wrong?

    I think iocontext allocated for a certain cgroup should be owned by the
    cgroup itself, whose code isn't implemented yet. I think you need to
    enhance it a bit if the owner is a cgroup.

    > > +retry:
    > > + if (!idr_pre_get(&bio_cgroup_id, GFP_KERNEL)) {
    > > + ret = -EAGAIN;
    > > + goto out_err;
    > > + }
    > > + spin_lock_irq(&bio_cgroup_idr_lock);
    > > + ret = idr_get_new_above(&bio_cgroup_id, (void *)biog, 1, &biog->id);
    > > + spin_unlock_irq(&bio_cgroup_idr_lock);
    > > + if (ret == -EAGAIN)
    > > + goto retry;
    > > + else if (ret)
    > > + goto out_err;
    > > +
    > > + return &biog->css;
    > > +out_err:
    > > + if (biog)
    > > + kfree(biog);
    > > + if (ioc)
    > > + put_io_context(ioc);
    > > + return ERR_PTR(ret);
    > > +}
    > > +
    > > +/* Delete the bio-cgroup. */
    > > +static void bio_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
    > > +{
    > > + struct bio_cgroup *biog = cgroup_bio(cgrp);
    > > +
    > > + put_io_context(biog->io_context);
    >
    > Here, I suspects what will happen under the following condition:
    > biog->io_context->refcount: 1 --> 0
    > biog->io_context->nr_tasks: 1 --> 1
    > ==> cfq_dtor() will be called but cfq_exit() has not be called yet.
    >
    > > +
    > > + spin_lock_irq(&bio_cgroup_idr_lock);
    > > + idr_remove(&bio_cgroup_id, biog->id);
    > > + spin_unlock_irq(&bio_cgroup_idr_lock);
    > > +
    > > + kfree(biog);
    > > +}
    > > +
    >
    > > +
    > > +/* Determine the iocontext of the bio-cgroup that issued a given bio. */
    > > +struct io_context *get_bio_cgroup_iocontext(struct bio *bio)
    > > +{
    > > + struct bio_cgroup *biog = NULL;
    > > + struct io_context *ioc;
    > > + int id = 0;
    > > +
    > > + id = get_bio_cgroup_id(bio);
    > > + if (id)
    > > + biog = find_bio_cgroup(id);
    > > + if (!biog)
    > > + biog = &default_bio_cgroup;
    > > + ioc = biog->io_context; /* default io_context for this cgroup */
    > > + atomic_inc(&ioc->refcount);
    > > + return ioc;
    > > +}
    > > +EXPORT_SYMBOL(get_bio_cgroup_iocontext);
    >
    >
    > Thanks,
    > Takuya Yoshikawa

    Thank you,
    Hirokazu Takahashi.


    \
     
     \ /
      Last update: 2008-11-16 12:49    [W:0.035 / U:38.612 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site