[03/12] BUG: Losing bits on request.cmd_flags

Commit Message

Shaun Tancheff April 4, 2016, 5:06 a.m. UTC

In a few places a temporary value smaller than a cmd_flags
is used to test for bits and or build up a new cmd_flags.

Change to use explicit u64 values where appropriate.

Signed-off-by: Shaun Tancheff <[e-mail]>
---
 block/blk-core.c         | 17 ++++++++++-------
 block/blk-merge.c        |  2 +-
 block/blk-mq.c           |  2 +-
 block/cfq-iosched.c      |  2 +-
 block/elevator.c         |  4 ++--
 include/linux/elevator.h |  4 ++--
 6 files changed, 17 insertions(+), 14 deletions(-)

Comments

Jeff Moyer April 4, 2016, 3:56 p.m. UTC | #1

Shaun Tancheff <[e-mail]> writes:

> In a few places a temporary value smaller than a cmd_flags
> is used to test for bits and or build up a new cmd_flags.
>
> Change to use explicit u64 values where appropriate.

This is not a bug fix, so please fix your subject.

I'm not against cleaning up the mixing of 32 vs 64 bit variables, but if
you're going to go down that path, then you might as well fix the mixing
of signed vs unsigned use as well.  And now that I look at it,
bio->bi_rw is unsigned long whereas req->cmd_flags is u64.  That could
make for fun bugs in the future.  We should at least add a comment in
the rq_flag_bits enum to the effect of bio flags need to stay in the
bottom 32 bits.

Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shaun Tancheff April 4, 2016, 4:06 p.m. UTC | #2

On Mon, Apr 4, 2016 at 10:56 PM, Jeff Moyer <[e-mail]> wrote:
> Shaun Tancheff <[e-mail]> writes:
>
>> In a few places a temporary value smaller than a cmd_flags
>> is used to test for bits and or build up a new cmd_flags.
>>
>> Change to use explicit u64 values where appropriate.
>
> This is not a bug fix, so please fix your subject.

Oh, okay. It became an issue in the next patch where some internal cmd_flags
temporaries where stored in 32 bit values.

I will fix patch/subject.

Thanks!

> I'm not against cleaning up the mixing of 32 vs 64 bit variables, but if
> you're going to go down that path, then you might as well fix the mixing
> of signed vs unsigned use as well.  And now that I look at it,
> bio->bi_rw is unsigned long whereas req->cmd_flags is u64.  That could
> make for fun bugs in the future.  We should at least add a comment in
> the rq_flag_bits enum to the effect of bio flags need to stay in the
> bottom 32 bits.
>
> Cheers,
> Jeff

Martin K. Petersen April 5, 2016, 2:08 a.m. UTC | #3

>>>>> "Shaun" == Shaun Tancheff <[e-mail]> writes:

Shaun> Oh, okay. It became an issue in the next patch where some
Shaun> internal cmd_flags temporaries where stored in 32 bit values.

Shaun> I will fix patch/subject.

There's a pending patch set from Mike Christie that cleans all this
up. Please use that as baseline.

Shaun Tancheff April 5, 2016, 10:39 a.m. UTC | #4

On Tue, Apr 5, 2016 at 9:08 AM, Martin K. Petersen
<[e-mail]> wrote:
>>>>>> "Shaun" == Shaun Tancheff <[e-mail]> writes:
>
> Shaun> Oh, okay. It became an issue in the next patch where some
> Shaun> internal cmd_flags temporaries where stored in 32 bit values.
>
> Shaun> I will fix patch/subject.
>
> There's a pending patch set from Mike Christie that cleans all this
> up. Please use that as baseline.

Given Mike Christie's patches this is not needed.
Thanks!

> --
> Martin K. Petersen      Oracle Linux Engineering

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index e4b1269..3413604 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -959,7 +959,7 @@  static void __freed_request(struct request_list *rl, int sync)
  * A request has just been released.  Account for it, update the full and
  * congestion status, wake up any waiters.   Called under q->queue_lock.
  */
-static void freed_request(struct request_list *rl, unsigned int flags)
+static void freed_request(struct request_list *rl, u64 flags)
 {
 	struct request_queue *q = rl->q;
 	int sync = rw_is_sync(flags);
@@ -1054,7 +1054,7 @@  static struct io_context *rq_ioc(struct bio *bio)
 /**
  * __get_request - get a free request
  * @rl: request list to allocate from
- * @rw_flags: RW and SYNC flags
+ * @rw: RW and SYNC flags
  * @bio: bio to allocate request for (can be %NULL)
  * @gfp_mask: allocation mask
  *
@@ -1065,7 +1065,7 @@  static struct io_context *rq_ioc(struct bio *bio)
  * Returns ERR_PTR on failure, with @q->queue_lock held.
  * Returns request pointer on success, with @q->queue_lock *not held*.
  */
-static struct request *__get_request(struct request_list *rl, int rw_flags,
+static struct request *__get_request(struct request_list *rl, unsigned long rw,
 				     struct bio *bio, gfp_t gfp_mask)
 {
 	struct request_queue *q = rl->q;
@@ -1073,6 +1073,7 @@  static struct request *__get_request(struct request_list *rl, int rw_flags,
 	struct elevator_type *et = q->elevator->type;
 	struct io_context *ioc = rq_ioc(bio);
 	struct io_cq *icq = NULL;
+	u64 rw_flags = rw;
 	const bool is_sync = rw_is_sync(rw_flags) != 0;
 	int may_queue;
 
@@ -1237,7 +1238,8 @@  rq_starved:
  * Returns ERR_PTR on failure, with @q->queue_lock held.
  * Returns request pointer on success, with @q->queue_lock *not held*.
  */
-static struct request *get_request(struct request_queue *q, int rw_flags,
+static struct request *get_request(struct request_queue *q,
+				   unsigned long rw_flags,
 				   struct bio *bio, gfp_t gfp_mask)
 {
 	const bool is_sync = rw_is_sync(rw_flags) != 0;
@@ -1490,7 +1492,7 @@  void __blk_put_request(struct request_queue *q, struct request *req)
 	 * it didn't come out of our reserved rq pools
 	 */
 	if (req->cmd_flags & REQ_ALLOCED) {
-		unsigned int flags = req->cmd_flags;
+		u64 flags = req->cmd_flags;
 		struct request_list *rl = blk_rq_rl(req);
 
 		BUG_ON(!list_empty(&req->queuelist));
@@ -1711,7 +1713,8 @@  static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
 {
 	const bool sync = !!(bio->bi_rw & REQ_SYNC);
 	struct blk_plug *plug;
-	int el_ret, rw_flags, where = ELEVATOR_INSERT_SORT;
+	u64 rw_flags;
+	int el_ret, where = ELEVATOR_INSERT_SORT;
 	struct request *req;
 	unsigned int request_count = 0;
 
@@ -2248,7 +2251,7 @@  EXPORT_SYMBOL_GPL(blk_insert_cloned_request);
  */
 unsigned int blk_rq_err_bytes(const struct request *rq)
 {
-	unsigned int ff = rq->cmd_flags & REQ_FAILFAST_MASK;
+	u64 ff = rq->cmd_flags & REQ_FAILFAST_MASK;
 	unsigned int bytes = 0;
 	struct bio *bio;
 
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 2613531..fec37e1 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -604,7 +604,7 @@  static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
  */
 void blk_rq_set_mixed_merge(struct request *rq)
 {
-	unsigned int ff = rq->cmd_flags & REQ_FAILFAST_MASK;
+	u64 ff = rq->cmd_flags & REQ_FAILFAST_MASK;
 	struct bio *bio;
 
 	if (rq->cmd_flags & REQ_MIXED_MERGE)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 56c0a72..8b41ad6 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -159,7 +159,7 @@  bool blk_mq_can_queue(struct blk_mq_hw_ctx *hctx)
 EXPORT_SYMBOL(blk_mq_can_queue);
 
 static void blk_mq_rq_ctx_init(struct request_queue *q, struct blk_mq_ctx *ctx,
-			       struct request *rq, unsigned int rw_flags)
+			       struct request *rq, u64 rw_flags)
 {
 	if (blk_queue_io_stat(q))
 		rw_flags |= REQ_IO_STAT;
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 1f9093e..e11a220 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -4262,7 +4262,7 @@  static inline int __cfq_may_queue(struct cfq_queue *cfqq)
 	return ELV_MQUEUE_MAY;
 }
 
-static int cfq_may_queue(struct request_queue *q, int rw)
+static int cfq_may_queue(struct request_queue *q, u64 rw)
 {
 	struct cfq_data *cfqd = q->elevator->elevator_data;
 	struct task_struct *tsk = current;
diff --git a/block/elevator.c b/block/elevator.c
index c3555c9..7c0a59c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -511,7 +511,7 @@  void elv_merge_requests(struct request_queue *q, struct request *rq,
 			     struct request *next)
 {
 	struct elevator_queue *e = q->elevator;
-	const int next_sorted = next->cmd_flags & REQ_SORTED;
+	const int next_sorted = !!(next->cmd_flags & REQ_SORTED);
 
 	if (next_sorted && e->type->ops.elevator_merge_req_fn)
 		e->type->ops.elevator_merge_req_fn(q, rq, next);
@@ -717,7 +717,7 @@  void elv_put_request(struct request_queue *q, struct request *rq)
 		e->type->ops.elevator_put_req_fn(rq);
 }
 
-int elv_may_queue(struct request_queue *q, int rw)
+int elv_may_queue(struct request_queue *q, u64 rw)
 {
 	struct elevator_queue *e = q->elevator;
 
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 638b324..a06cca4 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -26,7 +26,7 @@  typedef int (elevator_dispatch_fn) (struct request_queue *, int);
 typedef void (elevator_add_req_fn) (struct request_queue *, struct request *);
 typedef struct request *(elevator_request_list_fn) (struct request_queue *, struct request *);
 typedef void (elevator_completed_req_fn) (struct request_queue *, struct request *);
-typedef int (elevator_may_queue_fn) (struct request_queue *, int);
+typedef int (elevator_may_queue_fn) (struct request_queue *, u64);
 
 typedef void (elevator_init_icq_fn) (struct io_cq *);
 typedef void (elevator_exit_icq_fn) (struct io_cq *);
@@ -134,7 +134,7 @@  extern struct request *elv_former_request(struct request_queue *, struct request
 extern struct request *elv_latter_request(struct request_queue *, struct request *);
 extern int elv_register_queue(struct request_queue *q);
 extern void elv_unregister_queue(struct request_queue *q);
-extern int elv_may_queue(struct request_queue *, int);
+extern int elv_may_queue(struct request_queue *, u64);
 extern void elv_completed_request(struct request_queue *, struct request *);
 extern int elv_set_request(struct request_queue *q, struct request *rq,
 			   struct bio *bio, gfp_t gfp_mask);