block bits

GIT PULL In review Jens Axboe 3 participants 8 messages Feb 2, 2009 – Feb 8, 2009 2 messages not in the archive

Subsystems (4)

Documentation SCSI Block layer BSG (block layer generic sg v4 driver)

lore

Share & tools

lore

Jens Axboe

author

2009-02-02 13:17 UTC #0 lore raw

Hi Linus,

One critical bit in here, which fixes a reported oops from the iostat patch. The others are either trivial/minor or documentation. Please pull.

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

Alberto Bertogli (2):
Fix misleading comment in bio.h
bio.h: If they MUST be inlined, then use __always_inline

Jens Axboe (3):
block: fix oops in blk_queue_io_stat()
block: fix inconsistent parenthesisation of QUEUE_FLAG_DEFAULT
block: add text file detailing queue/ sysfs files

 Documentation/block/queue-sysfs.txt |   63 +++++++++++++++++++++++++++++++++++
 block/blk-core.c                    |    6 ++--
 block/blk.h                         |    8 ++++
 include/linux/bio.h                 |   10 +++--
 include/linux/blkdev.h              |    2 +-
 5 files changed, 81 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/block/queue-sysfs.txt

Documentation/block/queue-sysfs.txt new file +63 -0

new file mode 100644
index 0000000..e164403
@@ -0,0 +1,63 @@
+Queue sysfs files
+=================
+
+This text file will detail the queue files that are located in the sysfs tree
+for each block device. Note that stacked devices typically do not export
+any settings, since their queue merely functions are a remapping target.
+These files are the ones found in the /sys/block/xxx/queue/ directory.
+
+Files denoted with a RO postfix are readonly and the RW postfix means
+read-write.
+
+hw_sector_size (RO)
+-------------------
+This is the hardware sector size of the device, in bytes.
+
+max_hw_sectors_kb (RO)
+----------------------
+This is the maximum number of kilobytes supported in a single data transfer.
+
+max_sectors_kb (RW)
+-------------------
+This is the maximum number of kilobytes that the block layer will allow
+for a filesystem request. Must be smaller than or equal to the maximum
+size allowed by the hardware.
+
+nomerges (RW)
+-------------
+This enables the user to disable the lookup logic involved with IO merging
+requests in the block layer. Merging may still occur through a direct
+1-hit cache, since that comes for (almost) free. The IO scheduler will not
+waste cycles doing tree/hash lookups for merges if nomerges is 1. Defaults
+to 0, enabling all merges.
+
+nr_requests (RW)
+----------------
+This controls how many requests may be allocated in the block layer for
+read or write requests. Note that the total allocated number may be twice
+this amount, since it applies only to reads or writes (not the accumulated
+sum).
+
+read_ahead_kb (RW)
+------------------
+Maximum number of kilobytes to read-ahead for filesystems on this block
+device.
+
+rq_affinity (RW)
+----------------
+If this option is enabled, the block layer will migrate request completions
+to the CPU that originally submitted the request. For some workloads
+this provides a significant reduction in CPU cycles due to caching effects.
+
+scheduler (RW)
+--------------
+When read, this file will display the current and available IO schedulers
+for this block device. The currently active IO scheduler will be enclosed
+in [] brackets. Writing an IO scheduler name to this file will switch
+control of this block device to that new IO scheduler. Note that writing
+an IO scheduler name to this file will attempt to load that IO scheduler
+module, if it isn't already present in the system.
+
+
+
+Jens Axboe <[email protected]>, February 2009

block/blk-core.c +3 -3

index ca69f3d..29bcfac 100644
@@ -69,7 +69,7 @@ static void drive_stat_acct(struct request *rq, int new_io)
 	int rw = rq_data_dir(rq);
 	int cpu;
 
-	if (!blk_fs_request(rq) || !disk || !blk_queue_io_stat(disk->queue))
+	if (!blk_fs_request(rq) || !disk || !blk_do_io_stat(disk->queue))
 		return;
 
 	cpu = part_stat_lock();

@@ -1667,7 +1667,7 @@ static void blk_account_io_completion(struct request *req, unsigned int bytes)
 {
 	struct gendisk *disk = req->rq_disk;
 
-	if (!disk || !blk_queue_io_stat(disk->queue))
+	if (!disk || !blk_do_io_stat(disk->queue))
 		return;
 
 	if (blk_fs_request(req)) {

@@ -1686,7 +1686,7 @@ static void blk_account_io_done(struct request *req)
 {
 	struct gendisk *disk = req->rq_disk;
 
-	if (!disk || !blk_queue_io_stat(disk->queue))
+	if (!disk || !blk_do_io_stat(disk->queue))
 		return;
 
 	/*

block/blk.h +8 -0

index 6e1ed40..0dce92c 100644
@@ -108,4 +108,12 @@ static inline int blk_cpu_to_group(int cpu)
 #endif
 }
 
+static inline int blk_do_io_stat(struct request_queue *q)
+{
+	if (q)
+		return blk_queue_io_stat(q);
+
+	return 0;
+}
+
 #endif

include/linux/bio.h +6 -4

index 0942765..2aa283a 100644
@@ -451,12 +451,13 @@ extern struct biovec_slab bvec_slabs[BIOVEC_NR_POOLS] __read_mostly;
 
 #ifdef CONFIG_HIGHMEM
 /*
- * remember to add offset! and never ever reenable interrupts between a
- * bvec_kmap_irq and bvec_kunmap_irq!!
+ * remember never ever reenable interrupts between a bvec_kmap_irq and
+ * bvec_kunmap_irq!
  *
  * This function MUST be inlined - it plays with the CPU interrupt flags.
  */
-static inline char *bvec_kmap_irq(struct bio_vec *bvec, unsigned long *flags)
+static __always_inline char *bvec_kmap_irq(struct bio_vec *bvec,
+		unsigned long *flags)
 {
 	unsigned long addr;

@@ -472,7 +473,8 @@ static inline char *bvec_kmap_irq(struct bio_vec *bvec, unsigned long *flags)
 	return (char *) addr + bvec->bv_offset;
 }
 
-static inline void bvec_kunmap_irq(char *buffer, unsigned long *flags)
+static __always_inline void bvec_kunmap_irq(char *buffer,
+		unsigned long *flags)
 {
 	unsigned long ptr = (unsigned long) buffer & PAGE_MASK;

include/linux/blkdev.h +1 -1

index d08c4b8..dcaa0fd 100644
@@ -455,7 +455,7 @@ struct request_queue
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_CLUSTER) |		\
-				  1 << QUEUE_FLAG_STACKABLE)
+				 (1 << QUEUE_FLAG_STACKABLE))
 
 static inline int queue_is_locked(struct request_queue *q)
 {

Signature · 1 line

Jens Axboe

Boaz Harrosh

2009-02-02 17:31 UTC #1 lore raw

Quoted 8 lines · Jens Axboe

Hi Linus,
One critical bit in here, which fixes a reported oops from the iostat patch. The others are either trivial/minor or documentation. Please pull.
git://git.kernel.dk/linux-2.6-block.git for-linus

Quoted 9 lines

Alberto Bertogli (2):
Fix misleading comment in bio.h
bio.h: If they MUST be inlined, then use __always_inline
Jens Axboe (3):
block: fix oops in blk_queue_io_stat()
block: fix inconsistent parenthesisation of QUEUE_FLAG_DEFAULT
block: add text file detailing queue/ sysfs files

Hi Jens.

The bsg sense handling bug fix, is I think, a security breach in that Kernel copies dangling Kernel stack back to user-mode. It might even need a stable release.

At the time I was thinking that since bsg is marked EXPERIMENTAL in Kconfig then it must not be included in distro's Kernels by default, but Benny noted that it is included in Fedora since FC8 or FC9.

The patch I sent still applies I think?

Thanks
Boaz

Jens Axboe

author

2009-02-02 18:53 UTC #2 lore raw

Quoted 25 lines · Boaz Harrosh

Quoted 8 lines · Jens Axboe
Hi Linus,
One critical bit in here, which fixes a reported oops from the iostat patch. The others are either trivial/minor or documentation. Please pull.
git://git.kernel.dk/linux-2.6-block.git for-linus
Quoted 9 lines
Alberto Bertogli (2):
Fix misleading comment in bio.h
bio.h: If they MUST be inlined, then use __always_inline
Jens Axboe (3):
block: fix oops in blk_queue_io_stat()
block: fix inconsistent parenthesisation of QUEUE_FLAG_DEFAULT
block: add text file detailing queue/ sysfs files
Hi Jens.
The bsg sense handling bug fix, is I think, a security breach in that Kernel copies dangling Kernel stack back to user-mode. It might even need a stable release.

Quoted 5 lines

At the time I was thinking that since bsg is marked EXPERIMENTAL in Kconfig then it must not be included in distro's Kernels by default, but Benny noted that it is included in Fedora since FC8 or FC9.
The patch I sent still applies I think?

I'm sure it does, I didn't know if Tomo had swoopt it up himself. I'll add it to this branch.

Signature · 1 line

Jens Axboe

Boaz Harrosh

2009-02-03 06:34 UTC #3 lore raw

PATCH RESEND 1/1 bsg: Fix sense buffer bug in SG_IO

When submitting requests via SG_IO, which does a sync io, a bsg_command is not allocated. So an in-Kernel sense_buffer was not set. However when calling blk_execute_rq() with no sense buffer one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq() would check if rq->sense_len and a sense was requested by sg_io_v4 the rq->sense was copy_user() back, but by now it is already mangled stack memory.

I have fixed that by forcing a sense_buffer when calling bsg_map_hdr(). The bsg_command->sense is provided in the write/read path like before, and on-the-stack buffer is provided when doing SG_IO.

I have also fixed a dprintk message to print rq->errors in hex because of the scsi bit-field use of this member. For other block devices it does not matter anyway.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/bsg.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

block/bsg.c +10 -7

index d414bb5..0ce8806 100644
@@ -244,7 +244,8 @@ bsg_validate_sgv4_hdr(struct request_queue *q, struct sg_io_v4 *hdr, int *rw)
  * map sg_io_v4 to a request.
  */
 static struct request *
-bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm)
+bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
+	    u8 *sense)
 {
 	struct request_queue *q = bd->queue;
 	struct request *rq, *next_rq = NULL;

@@ -306,6 +307,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm)
 		if (ret)
 			goto out;
 	}
+
+	rq->sense = sense;
+	rq->sense_len = 0;
+
 	return rq;
 out:
 	if (rq->cmd != rq->__cmd)

@@ -348,9 +353,6 @@ static void bsg_rq_end_io(struct request *rq, int uptodate)
 static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
 			    struct bsg_command *bc, struct request *rq)
 {
-	rq->sense = bc->sense;
-	rq->sense_len = 0;
-
 	/*
 	 * add bc command to busy queue and submit rq for io
 	 */

@@ -419,7 +421,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 {
 	int ret = 0;
 
-	dprintk("rq %p bio %p %u\n", rq, bio, rq->errors);
+	dprintk("rq %p bio %p 0x%x\n", rq, bio, rq->errors);
 	/*
 	 * fill in all the output members
 	 */

@@ -635,7 +637,7 @@ static int __bsg_write(struct bsg_device *bd, const char __user *buf,
 		/*
 		 * get a request, fill in the blanks, and add to request queue
 		 */
-		rq = bsg_map_hdr(bd, &bc->hdr, has_write_perm);
+		rq = bsg_map_hdr(bd, &bc->hdr, has_write_perm, bc->sense);
 		if (IS_ERR(rq)) {
 			ret = PTR_ERR(rq);
 			rq = NULL;

@@ -922,11 +924,12 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		struct request *rq;
 		struct bio *bio, *bidi_bio = NULL;
 		struct sg_io_v4 hdr;
+		u8 sense[SCSI_SENSE_BUFFERSIZE];
 
 		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
 			return -EFAULT;
 
-		rq = bsg_map_hdr(bd, &hdr, file->f_mode & FMODE_WRITE);
+		rq = bsg_map_hdr(bd, &hdr, file->f_mode & FMODE_WRITE, sense);
 		if (IS_ERR(rq))
 			return PTR_ERR(rq);

Signature · 1 line

1.6.0.1

Jens Axboe

author

2009-02-03 06:50 UTC #4 lore raw

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

Quoted 9 lines · Boaz Harrosh

When submitting requests via SG_IO, which does a sync io, a bsg_command is not allocated. So an in-Kernel sense_buffer was not set. However when calling blk_execute_rq() with no sense buffer one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq() would check if rq->sense_len and a sense was requested by sg_io_v4 the rq->sense was copy_user() back, but by now it is already mangled stack memory.

Quoted 7 lines

I have fixed that by forcing a sense_buffer when calling bsg_map_hdr(). The bsg_command->sense is provided in the write/read path like before, and on-the-stack buffer is provided when doing SG_IO.
I have also fixed a dprintk message to print rq->errors in hex because of the scsi bit-field use of this member. For other block devices it does not matter anyway.

Thanks for the resend Boaz, I'll make sure it gets upstream for 2.6.29.

Signature · 1 line

Jens Axboe

FUJITA Tomonori

maintainer Acked-by on v1 1/1

2009-02-03 06:58 UTC #5 lore raw

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

In reply to Boaz Harrosh

Quoted 13 lines · On Tue, 03 Feb 2009 08:34:07 +0200 Boaz Harrosh <[email protected]> wrote:

When submitting requests via SG_IO, which does a sync io, a bsg_command is not allocated. So an in-Kernel sense_buffer was not set. However when calling blk_execute_rq() with no sense buffer one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq() would check if rq->sense_len and a sense was requested by sg_io_v4 the rq->sense was copy_user() back, but by now it is already mangled stack memory.
I have fixed that by forcing a sense_buffer when calling bsg_map_hdr(). The bsg_command->sense is provided in the write/read path like before, and on-the-stack buffer is provided when doing SG_IO.

Quoted 8 lines

I have also fixed a dprintk message to print rq->errors in hex because of the scsi bit-field use of this member. For other block devices it does not matter anyway.
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

Acked-by FUJITA Tomonori <[email protected]>

I think I did this in the previous submission though.

Jens Axboe

author

2009-02-03 07:06 UTC #6 lore raw

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

Quoted 25 lines · FUJITA Tomonori

Quoted 13 lines · On Tue, 03 Feb 2009 08:34:07 +0200 Boaz Harrosh <[email protected]> wrote:
When submitting requests via SG_IO, which does a sync io, a bsg_command is not allocated. So an in-Kernel sense_buffer was not set. However when calling blk_execute_rq() with no sense buffer one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq() would check if rq->sense_len and a sense was requested by sg_io_v4 the rq->sense was copy_user() back, but by now it is already mangled stack memory.
I have fixed that by forcing a sense_buffer when calling bsg_map_hdr(). The bsg_command->sense is provided in the write/read path like before, and on-the-stack buffer is provided when doing SG_IO.
Quoted 8 lines
I have also fixed a dprintk message to print rq->errors in hex because of the scsi bit-field use of this member. For other block devices it does not matter anyway.
Signed-off-by Boaz Harrosh <[email protected]>
---
 block/bsg.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

Quoted 4 lines

Acked-by FUJITA Tomonori <[email protected]>
I think I did this in the previous submission though.

Yeah, I also added it.

Signature · 1 line

Jens Axboe

Boaz Harrosh

2009-02-08 13:59 UTC #9 lore raw

Re: [osd-dev] Kernel crashes on login

In reply to a message not in the archive

Quoted 64 lines · Boaz Harrosh

Quoted 41 lines · Fuerst Lars
Hello Boaz,
I try to use your kernel initiator in combination with the OSC osd2 target. The target (FC 9, 2.6.26.5-45.fc9.x86_64) works fine with the user space initiator from osc, but when i use your initiator (in tree build 2.6.29-rc1-9) i get a failure after login:
# iscsiadm -m node -p 141.11.96.100 -T render01crh --login
Logging in to [iface: default, target: render01crh, portal:
141.11.96.100,3260]
iscsiadm: got read error (0/0), daemon died?
iscsiadm: Could not login to [iface: default, target: render01crh,
portal: 141.11.96.100,3260]:
iscsiadm: initiator reported error (18 - could not communicate to iscsid)
kernel log says:
[..]
Feb  5 14:29:16 render02crh kernel: osd:
OSD_ATTR_RI_VENDOR_IDENTIFICATION [<NULL>]
Feb  5 14:29:16 render02crh kernel: osd:
OSD_ATTR_RI_PRODUCT_IDENTIFICATION [<NULL>]
Feb  5 14:29:16 render02crh kernel: osd: OSD_ATTR_RI_PRODUCT_MODEL
[<NULL>]                                                             
Feb  5 14:29:16 render02crh kernel: BUG: unable to handle kernel NULL
pointer dereference at (null)                                     
Feb  5 14:29:16 render02crh kernel: IP: [<ffffffffa3114aa3>]
_osd_print_system_info+0x177/0x29b [libosd]
Feb  5 14:29:16 render02crh kernel: PGD 6ad4d067 PUD 785d5067 PMD 0
Feb  5 14:29:16 render02crh kernel: Oops: 0000 [#1] SMP
Feb  5 14:29:16 render02crh kernel: last sysfs file:
/sys/devices/platform/host5/scsi_host/host5/scan
[..]
Quoted 8 lines
Complete log and kernel config is attached. It seems to be a problem during the osd auto detection. Do you have an idea?
Thanks in advance
Regards,
Lars
Thanks Lars, It should not crash, I'll fix that ASAP
But please use the OSC's target from the open-osd.org git trees. Please follow the instructions in this page:
http://www.open-osd.org/bin/view/Main/OscOsdProject
Specifically the "How to get started" section. This will also automatically fetch the tgt module with all the right patches applied. So it should be easier to use.
The original version is missing some mandatory attributes and other incompatibilities with OSD2 been crafted on draft 01, when currently we're on draft 04d.

Quoted 7 lines

But sorry I should have checked that. Code should not crash ever.
I'll send a fix today.
Boaz

Nice things about having some users is that they get to find your bugs ;) . Thanks Lars

Some project History:

At git://git.open-osd.org/osc-osd, our OSC's osd-target git tree,
there are a couple of fixes and enhancements over the latest source code from OSC's internal SVN.

Prior to that when we first started to use OSC's target we sent to
Pete and Anath fixes regarding none conforming behavior with regard to attributes. The tar balls that where public on the net at the time had these short comings.

Lars are you using the old tar balls, I thought they where removed from the net around the time Pete left OSC. (By accident)

The latest SVN code included our fixes, (and did not fully compile). Are only publicly available on above git in the "osc" branch. I have tested with that version and it does not crash so it might be that you are using the old tar balls. In that case even with my patch to fix the crash, attributes will not work at all and you will not be able to work with open-osd initiator. :-(

It is strongly recommended that you upgrade to the open-osd version of OSC's osd-target. (It also includes a matching OSC's osd-initiator)

I'm sending a patch as it's own email thread to James for inclusion into 2.6.30.

Thank you for trying out and helping with the open-osd initiator.

Boaz