lkml.org 
[lkml]   [2009]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 04/11] writeback: switch to per-bdi threads for flushing data
      The patch set seems easier to read now. Thanks for cleaning it up.

    > +void bdi_writeback_all(struct super_block *sb, struct writeback_control *wbc)
    > +{
    > + struct backing_dev_info *bdi, *tmp;
    > +
    > + mutex_lock(&bdi_lock);
    > +
    > + list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) {
    > + if (!bdi_has_dirty_io(bdi))
    > + continue;
    > + bdi_start_writeback(bdi, sb, wbc->nr_to_write, wbc->sync_mode);
    > + }
    > +
    > + mutex_unlock(&bdi_lock);
    > +}
    > +
    Looking at this function, I've realized that wbc->nr_to_write has a bit
    silly meaning here. Each BDI will be kicked to write nr_to_write pages
    which is not what it used to mean originally. I don't think it really matters
    but we should have this in mind...

    > @@ -591,13 +715,10 @@ static void generic_sync_bdi_inodes(struct backing_dev_info *bdi,
    > void generic_sync_sb_inodes(struct super_block *sb,
    > struct writeback_control *wbc)
    > {
    > - const int is_blkdev_sb = sb_is_blkdev_sb(sb);
    > - struct backing_dev_info *bdi;
    > -
    > - mutex_lock(&bdi_lock);
    > - list_for_each_entry(bdi, &bdi_list, bdi_list)
    > - generic_sync_bdi_inodes(bdi, wbc, sb, is_blkdev_sb);
    > - mutex_unlock(&bdi_lock);
    > + if (wbc->bdi)
    > + generic_sync_bdi_inodes(sb, wbc);
    > + else
    > + bdi_writeback_all(sb, wbc);
    I guess this asynchronousness is just transient...

    > +static int bdi_forker_task(void *ptr)
    > +{
    > + struct backing_dev_info *me = ptr;
    > + DEFINE_WAIT(wait);
    > +
    > + for (;;) {
    > + struct backing_dev_info *bdi, *tmp;
    > +
    > + /*
    > + * Do this periodically, like kupdated() did before.
    > + */
    > + sync_supers();
    Ugh, this looks nasty. Moreover I'm afraid of forker_task() getting stuck
    (and thus not being able to start new threads) in sync_supers() when some
    fs is busy and other needs to create flusher thread...
    Why not just having a separate thread for this? I know we have lots of
    kernel threads already but this one seems like a useful one... Or do you
    plan getting rid of this completely sometime in the near future and sync
    supers also from per-bdi thread (which would make a lot of sence to me)?

    > +
    > + /*
    > + * Temporary measure, we want to make sure we don't see
    > + * dirty data on the default backing_dev_info
    > + */
    > + if (bdi_has_dirty_io(me))
    > + bdi_flush_io(me);
    > +
    > + prepare_to_wait(&me->wait, &wait, TASK_INTERRUPTIBLE);
    > +
    > + mutex_lock(&bdi_lock);
    > +
    > + /*
    > + * Check if any existing bdi's have dirty data without
    > + * a thread registered. If so, set that up.
    > + */
    > + list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) {
    > + if (bdi->task || !bdi_has_dirty_io(bdi))
    > + continue;
    > +
    > + bdi_add_default_flusher_task(bdi);
    > + }
    > +
    > + if (list_empty(&bdi_pending_list)) {
    > + unsigned long wait;
    > +
    > + mutex_unlock(&bdi_lock);
    > + wait = msecs_to_jiffies(dirty_writeback_interval * 10);
    > + schedule_timeout(wait);
    > + try_to_freeze();
    > + continue;
    > + }
    > +
    > + /*
    > + * This is our real job - check for pending entries in
    > + * bdi_pending_list, and create the tasks that got added
    > + */
    > + bdi = list_entry(bdi_pending_list.next, struct backing_dev_info,
    > + bdi_list);
    > + list_del_init(&bdi->bdi_list);
    > + mutex_unlock(&bdi_lock);
    > +
    > + BUG_ON(bdi->task);
    > +
    > + bdi->task = kthread_run(bdi_start_fn, bdi, "bdi-%s",
    > + dev_name(bdi->dev));
    > + /*
    > + * If task creation fails, then readd the bdi to
    > + * the pending list and force writeout of the bdi
    > + * from this forker thread. That will free some memory
    > + * and we can try again.
    > + */
    > + if (!bdi->task) {
    > + /*
    > + * Add this 'bdi' to the back, so we get
    > + * a chance to flush other bdi's to free
    > + * memory.
    > + */
    > + mutex_lock(&bdi_lock);
    > + list_add_tail(&bdi->bdi_list, &bdi_pending_list);
    > + mutex_unlock(&bdi_lock);
    > +
    > + bdi_flush_io(bdi);
    > + }
    > + }
    > +
    > + finish_wait(&me->wait, &wait);
    > + return 0;
    > +}

    Honza
    --
    Jan Kara <jack@suse.cz>
    SUSE Labs, CR


    \
     
     \ /
      Last update: 2009-05-27 17:17    [W:0.031 / U:90.692 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site