Skip to contentSkip to conversation

block fixes for 2.6.36-rc5

GIT PULL Jens Axboe 2 participants 3 messages Sep 22, 2010

Subsystems (5)
Memory management VFS Block layer Char and misc drivers +1 more subsystems
lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

Hi Linus,

A small batch of fixes. Two important fixes, correcting a bug in the unified request/bio command flags causing blk_rq_map_kern() to flag writes incorrectly, and the oops in CFQ with cgroups and a usb key. The small 3-patch series from Jan fixes a long standing regression dating back to 2.6.32.

Please pull.

git://git.kernel.dk/linux-2.6-block.git for-linus

Benny Halevy (1):
block: fix blk_rq_map_kern bio direction flag

Dan Carpenter (1):
cciss: freeing uninitialized data on error path

Jan Kara (3):
bdi: Initialize noop_backing_dev_info properly
char: Mark /dev/zero and /dev/kmem as not capable of writeback
bdi: Fix warnings in __mark_inode_dirty for /dev/zero and friends

Vivek Goyal (1):
cfq-iosched: fix a kernel OOPs when usb key is inserted

 block/blk-map.c       |    2 +-
 block/cfq-iosched.c   |   16 +++++++++++++---
 drivers/block/cciss.c |    2 +-
 drivers/char/mem.c    |    3 ++-
 fs/char_dev.c         |    4 +++-
 fs/fs-writeback.c     |   23 +++++++++++++++++++++--
 mm/backing-dev.c      |    2 ++
 7 files changed, 43 insertions(+), 9 deletions(-)
block/blk-map.c +1 -1
index c65d759..ade0a08 100644
@@ -307,7 +307,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 		return PTR_ERR(bio);
 
 	if (rq_data_dir(rq) == WRITE)
-		bio->bi_rw |= (1 << REQ_WRITE);
+		bio->bi_rw |= REQ_WRITE;
 
 	if (do_copy)
 		rq->cmd_flags |= REQ_COPY_USER;
block/cfq-iosched.c +13 -3
index f65c6f0..9eba291 100644
@@ -1019,10 +1019,20 @@ cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create)
 	 */
 	atomic_set(&cfqg->ref, 1);
 
-	/* Add group onto cgroup list */
-	sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
-	cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+	/*
+	 * Add group onto cgroup list. It might happen that bdi->dev is
+	 * not initiliazed yet. Initialize this new group without major
+	 * and minor info and this info will be filled in once a new thread
+	 * comes for IO. See code above.
+	 */
+	if (bdi->dev) {
+		sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
+		cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
 					MKDEV(major, minor));
+	} else
+		cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+					0);
+
 	cfqg->weight = blkcg_get_weight(blkcg, cfqg->blkg.dev);
 
 	/* Add group on cfqd list */
drivers/block/cciss.c +1 -1
index 6124c2f..5e4fadc 100644
@@ -4792,7 +4792,7 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 clean4:
 	kfree(h->cmd_pool_bits);
 	/* Free up sg elements */
-	for (k = 0; k < h->nr_cmds; k++)
+	for (k-- ; k >= 0; k--)
 		kfree(h->scatter_list[k]);
 	kfree(h->scatter_list);
 	cciss_free_sg_chain_blocks(h->cmd_sg_list, h->nr_cmds);
drivers/char/mem.c +2 -1
index a398ecd..1f528fa 100644
@@ -788,10 +788,11 @@ static const struct file_operations zero_fops = {
 /*
  * capabilities for /dev/zero
  * - permits private mappings, "copies" are taken of the source of zeros
+ * - no writeback happens
  */
 static struct backing_dev_info zero_bdi = {
 	.name		= "char/mem",
-	.capabilities	= BDI_CAP_MAP_COPY,
+	.capabilities	= BDI_CAP_MAP_COPY | BDI_CAP_NO_ACCT_AND_WRITEBACK,
 };
 
 static const struct file_operations full_fops = {
fs/char_dev.c +3 -1
index f80a4f2..143d393 100644
@@ -40,7 +40,9 @@ struct backing_dev_info directly_mappable_cdev_bdi = {
 #endif
 		/* permit direct mmap, for read, write or exec */
 		BDI_CAP_MAP_DIRECT |
-		BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP),
+		BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP |
+		/* no writeback happens */
+		BDI_CAP_NO_ACCT_AND_WRITEBACK),
 };
 
 static struct kobj_map *cdev_map;
fs/fs-writeback.c +21 -2
index 81e086d..5581122 100644
@@ -52,8 +52,6 @@ struct wb_writeback_work {
 #define CREATE_TRACE_POINTS
 #include <trace/events/writeback.h>
 
-#define inode_to_bdi(inode)	((inode)->i_mapping->backing_dev_info)
-
 /*
  * We don't actually have pdflush, but this one is exported though /proc...
  */

@@ -71,6 +69,27 @@ int writeback_in_progress(struct backing_dev_info *bdi)
 	return test_bit(BDI_writeback_running, &bdi->state);
 }
 
+static inline struct backing_dev_info *inode_to_bdi(struct inode *inode)
+{
+	struct super_block *sb = inode->i_sb;
+	struct backing_dev_info *bdi = inode->i_mapping->backing_dev_info;
+
+	/*
+	 * For inodes on standard filesystems, we use superblock's bdi. For
+	 * inodes on virtual filesystems, we want to use inode mapping's bdi
+	 * because they can possibly point to something useful (think about
+	 * block_dev filesystem).
+	 */
+	if (sb->s_bdi && sb->s_bdi != &noop_backing_dev_info) {
+		/* Some device inodes could play dirty tricks. Catch them... */
+		WARN(bdi != sb->s_bdi && bdi_cap_writeback_dirty(bdi),
+			"Dirtiable inode bdi %s != sb bdi %s\n",
+			bdi->name, sb->s_bdi->name);
+		return sb->s_bdi;
+	}
+	return bdi;
+}
+
 static void bdi_queue_work(struct backing_dev_info *bdi,
 		struct wb_writeback_work *work)
 {
mm/backing-dev.c +2 -0
index c2bf86f..65d4204 100644
@@ -30,6 +30,7 @@ EXPORT_SYMBOL_GPL(default_backing_dev_info);
 
 struct backing_dev_info noop_backing_dev_info = {
 	.name		= "noop",
+	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
 };
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 

@@ -243,6 +244,7 @@ static int __init default_bdi_init(void)
 	err = bdi_init(&default_backing_dev_info);
 	if (!err)
 		bdi_register(&default_backing_dev_info, NULL, "default");
+	err = bdi_init(&noop_backing_dev_info);
 
 	return err;
 }
Signature · 1 line
Jens Axboe
#1 lore raw

Gaah. This is just _incredibly_ ugly:

Quoted 10 lines · Jens Axboe

-       /* Add group onto cgroup list */
-       sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
-       cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+       /*
+        * Add group onto cgroup list. It might happen that bdi->dev is
+        * not initiliazed yet. Initialize this new group without major
+        * and minor info and this info will be filled in once a new thread
+        * comes for IO. See code above.
+        */

Quoted 8 lines

+       if (bdi->dev) {
+               sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
+               cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd, MKDEV(major, minor));
+       } else
+               cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+                                       0);
+

and quite frankly, anything that does that kind of thing is total sh*t. Not only is the sscanf() just broken (really? figuring out things from some internal string? Using dev_t in this time and age for kernel internal stuff?) to begin with, but if you have to then do it conditionally, for chrissake do it _cleanly_.

Make a small helper function that does "get me the dev_t of this 'dev'", and make that one do

   static unsigned int device_dev_t(struct device *dev)
   {
      unsigned int major = 0, minor = 0;
      if (dev)
         sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);

      return MKDEV(major, minor);

}

and then just have a single 'cfq_blkiocg_add_blkio_group()' there.

But more seriously, why the hell does anything internal to cfq use a 'dev_t' in the first place? Why isn't that 'struct blkio_group' using a pointer to the 'struct device' or something like that instead (or the pointer to the queue, or whatever)? It's just damn wrong to use dev_t in this day and age, and the fact that you need to make it up using sscanf() should have clued people into that fact.

I hate seeing obvious crap-workarounds this late in an -rc.

Linus

author
#2 lore raw
Quoted 43 lines · Linus Torvalds

Gaah. This is just _incredibly_ ugly:

Quoted 8 lines · Jens Axboe
block/cfq-iosched.c +4 -3
-       /* Add group onto cgroup list */
-       sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
-       cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+       /*
+        * Add group onto cgroup list. It might happen that bdi->dev is
+        * not initiliazed yet. Initialize this new group without major
+        * and minor info and this info will be filled in once a new thread

block/cfq-iosched.c cfq_find_alloc_cfqg(struct cfq_data *cfqd, struct cgroup *cgroup, int create) · line 1035

Quoted 10 lines
block/cfq-iosched.c +9 -0
+        * comes for IO. See code above.
+        */
+       if (bdi->dev) {
+               sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);
+               cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
                                       MKDEV(major, minor));
+       } else
+               cfq_blkiocg_add_blkio_group(blkcg, &cfqg->blkg, (void *)cfqd,
+                                       0);
+

and quite frankly, anything that does that kind of thing is total sh*t. Not only is the sscanf() just broken (really? figuring out things from some internal string? Using dev_t in this time and age for kernel internal stuff?) to begin with, but if you have to then do it conditionally, for chrissake do it _cleanly_.

Make a small helper function that does "get me the dev_t of this 'dev'", and make that one do

   static unsigned int device_dev_t(struct device *dev)
   {
      unsigned int major = 0, minor = 0;
      if (dev)
         sscanf(dev_name(bdi->dev), "%u:%u", &major, &minor);

      return MKDEV(major, minor);

}

and then just have a single 'cfq_blkiocg_add_blkio_group()' there.

Quoted 8 lines

But more seriously, why the hell does anything internal to cfq use a 'dev_t' in the first place? Why isn't that 'struct blkio_group' using a pointer to the 'struct device' or something like that instead (or the pointer to the queue, or whatever)? It's just damn wrong to use dev_t in this day and age, and the fact that you need to make it up using sscanf() should have clued people into that fact.

I hate seeing obvious crap-workarounds this late in an -rc.

I don't think anybody will disagree. The major problem here is that we still have some drivers using multiple devices per queue, one of them will be gone for .37 at least and I hope we can get mtd blk converted too. When that happens, we can place a dev in the queue and get rid of this hack and similar hacks on double bdi etc registrations.

So we can put some makeup on the corpse like you describe, or we can just live with this until we can kill off abominations like this for 2.6.37.

Signature · 1 line
Jens Axboe