[Drbd-dev] [PATCH] - DRBD 8.0: Add standard disk counters to DRBD devices

[Drbd-dev] [PATCH] - DRBD 8.0: Add standard disk counters to DRBD devices

[Graham, Simon]

[-- Attachment #1.1: Type: text/plain, Size: 932 bytes --]

The attached patch adds support for the standard disk stats (as reported
in /proc/diskstats et al) at the DRBD device level - this is nice
because it can be used to show the actual response times etc at that
level, after both local and remote (and meta data) I/O is done. These
numbers also show up in the standard sar data for disk I/O.

 

It was a little trickier than I thought because getting the right values
for the various time fields is not obvious but in the end I copied what
md and ll_rw_blk.c do and the numbers certainly look correct now. There
are also a few error paths at the top of make_request that do not count
bio's that are failed early - since I need to hold the mdev request lock
when manipulating the counters, I put the code in after the lock was
already acquired which leaves a few error paths that are not counted.
However, the normal paths are there, so I think it's OK.

 

Simon


[-- Attachment #1.2: Type: text/html, Size: 2709 bytes --]

[-- Attachment #2: 0003-Add-support-for-standard-disk-stats-to-DRBD-devices.patch --]
[-- Type: application/octet-stream, Size: 3385 bytes --]

From 0b35a81ccda06c6711cdf3cbae1a0cff033ca25e Mon Sep 17 00:00:00 2001
From: Simon P. Graham <Simon.Graham@stratus.com>
Date: Mon, 14 Apr 2008 10:35:33 -0400
Subject: [PATCH] Add support for standard disk stats to DRBD devices

---
 drbd/drbd_int.h |    1 +
 drbd/drbd_req.c |   38 ++++++++++++++++++++++++++++++++++++++
 drbd/drbd_req.h |    1 +
 3 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/drbd/drbd_int.h b/drbd/drbd_int.h
index 8f693f3..05edc55 100644
--- a/drbd/drbd_int.h
+++ b/drbd/drbd_int.h
@@ -629,6 +629,7 @@ struct drbd_request {
 	struct bio *master_bio;       /* master bio pointer */
 	unsigned long rq_state; /* see comments above _req_mod() */
 	int seq_num;
+	unsigned long start_time;
 };
 
 struct drbd_barrier {
diff --git a/drbd/drbd_req.c b/drbd/drbd_req.c
index f06a0e4..d75b674 100644
--- a/drbd/drbd_req.c
+++ b/drbd/drbd_req.c
@@ -105,6 +105,37 @@ void _print_req_mod(drbd_request_t *req,drbd_req_event_t what)
 #define print_req_mod(T,W)
 #endif
 
+/* Update disk stats at start of I/O request */
+static inline void _drbd_start_io_acct(drbd_dev *mdev, drbd_request_t *req, struct bio *bio)
+{
+	int rw = bio_data_dir(bio);
+
+	MUST_HOLD(&mdev->req_lock)
+
+	disk_stat_inc(mdev->vdisk, ios[rw]);
+	disk_stat_add(mdev->vdisk, sectors[rw], bio_sectors(bio));
+	preempt_disable();
+	disk_round_stats(mdev->vdisk);
+	preempt_enable();
+	mdev->vdisk->in_flight++;
+}
+
+/* Update disk stats when completing request upwards */
+static inline void _drbd_end_io_acct(drbd_dev *mdev, drbd_request_t *req)
+{
+	int rw = bio_data_dir(req->master_bio);
+	unsigned long duration = jiffies - req->start_time;
+
+	MUST_HOLD(&mdev->req_lock)
+
+	preempt_disable();
+	disk_round_stats(mdev->vdisk);
+	preempt_enable();
+	mdev->vdisk->in_flight--;
+
+	disk_stat_add(mdev->vdisk, ticks[rw], duration);
+}
+
 static void _req_is_done(drbd_dev *mdev, drbd_request_t *req, const int rw)
 {
 	const unsigned long s = req->rq_state;
@@ -335,6 +366,9 @@ void _req_may_be_done(drbd_request_t *req, int error)
 		 * then again, if it is a READ, it is not in the TL at all.
 		 * is it still leagal to complete a READ during freeze? */
 
+		/* Update disk stats */
+		_drbd_end_io_acct(mdev, req);
+
 		_complete_master_bio(mdev,req,
 			  ok ? 0 : ( error ? error : -EIO ) );
 	} else {
@@ -956,6 +990,9 @@ drbd_make_request_common(drbd_dev *mdev, struct bio *bio)
 	}
 
 
+	/* Update disk stats */
+	_drbd_start_io_acct(mdev, req, bio);
+
 	/* _maybe_start_new_epoch(mdev);
 	 * If we need to generate a write barrier packet, we have to add the
 	 * new epoch (barrier) object, and queue the barrier packet for sending,
@@ -1012,6 +1049,7 @@ drbd_make_request_common(drbd_dev *mdev, struct bio *bio)
 			local = 0;
 		}
 		if (remote) dec_ap_pending(mdev);
+		_drbd_end_io_acct(mdev, req);
 		/* THINK: do we want to fail it (-EIO), or pretend success? */
 		bio_endio(req->master_bio, 0);
 		req->master_bio = NULL;
diff --git a/drbd/drbd_req.h b/drbd/drbd_req.h
index 1d981b2..74d8dc1 100644
--- a/drbd/drbd_req.h
+++ b/drbd/drbd_req.h
@@ -284,6 +284,7 @@ static inline drbd_request_t* drbd_req_new(drbd_dev *mdev, struct bio *bio_src)
 		req->epoch       = 0;
 		req->sector      = bio->bi_sector;
 		req->size        = bio->bi_size;
+		req->start_time  = jiffies;
 		INIT_HLIST_NODE(&req->colision);
 		INIT_LIST_HEAD(&req->tl_requests);
 
-- 
1.5.4-rc1.GIT

Re: [Drbd-dev] [PATCH] - DRBD 8.0: Add standard disk counters to DRBD devices

[Lars Ellenberg]

On Mon, Apr 14, 2008 at 02:58:26PM -0400, Graham, Simon wrote:
> The attached patch adds support for the standard disk stats (as reported
> in /proc/diskstats et al) at the DRBD device level - this is nice
> because it can be used to show the actual response times etc at that
> level, after both local and remote (and meta data) I/O is done. These
> numbers also show up in the standard sar data for disk I/O.

wonderful :)
had been somewhere on my todo list for month, 
never got around even to look at it, though.
thanks!

> It was a little trickier than I thought because getting the right values
> for the various time fields is not obvious but in the end I copied what
> md and ll_rw_blk.c do and the numbers certainly look correct now. There
> are also a few error paths at the top of make_request that do not count
> bio's that are failed early - since I need to hold the mdev request lock
> when manipulating the counters, I put the code in after the lock was
> already acquired which leaves a few error paths that are not counted.
> However, the normal paths are there, so I think it's OK.

that sounds perfectly reasonable.

Content-Description: 0003-Add-support-for-standard-disk-stats-to-DRBD-devices.patch
> From 0b35a81ccda06c6711cdf3cbae1a0cff033ca25e Mon Sep 17 00:00:00 2001
> From: Simon P. Graham <Simon.Graham@stratus.com>
> Date: Mon, 14 Apr 2008 10:35:33 -0400
> Subject: [PATCH] Add support for standard disk stats to DRBD devices
> 
> ---
>  drbd/drbd_int.h |    1 +
>  drbd/drbd_req.c |   38 ++++++++++++++++++++++++++++++++++++++
>  drbd/drbd_req.h |    1 +
>  3 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/drbd/drbd_int.h b/drbd/drbd_int.h
> index 8f693f3..05edc55 100644
> --- a/drbd/drbd_int.h
> +++ b/drbd/drbd_int.h
> @@ -629,6 +629,7 @@ struct drbd_request {
>  	struct bio *master_bio;       /* master bio pointer */
>  	unsigned long rq_state; /* see comments above _req_mod() */
>  	int seq_num;
> +	unsigned long start_time;
>  };

yes, structures keep growing ...
somewhen we need to clean it all up again.

>  struct drbd_barrier {
> diff --git a/drbd/drbd_req.c b/drbd/drbd_req.c
> index f06a0e4..d75b674 100644
> --- a/drbd/drbd_req.c
> +++ b/drbd/drbd_req.c
> @@ -105,6 +105,37 @@ void _print_req_mod(drbd_request_t *req,drbd_req_event_t what)
>  #define print_req_mod(T,W)
>  #endif
>  
> +/* Update disk stats at start of I/O request */
> +static inline void _drbd_start_io_acct(drbd_dev *mdev, drbd_request_t *req, struct bio *bio)
> +{
> +	int rw = bio_data_dir(bio);
> +
> +	MUST_HOLD(&mdev->req_lock)
> +
> +	disk_stat_inc(mdev->vdisk, ios[rw]);
> +	disk_stat_add(mdev->vdisk, sectors[rw], bio_sectors(bio));
> +	preempt_disable();

we are inside a spin_lock, no need to manipulate preempt,
there is no preemptible spin_lock yet.

> +	disk_round_stats(mdev->vdisk);
> +	preempt_enable();
> +	mdev->vdisk->in_flight++;
> +}
> +
> +/* Update disk stats when completing request upwards */
> +static inline void _drbd_end_io_acct(drbd_dev *mdev, drbd_request_t *req)
> +{
> +	int rw = bio_data_dir(req->master_bio);
> +	unsigned long duration = jiffies - req->start_time;
> +
> +	MUST_HOLD(&mdev->req_lock)
> +
> +	preempt_disable();

same here.

> +	disk_round_stats(mdev->vdisk);
> +	preempt_enable();
> +	mdev->vdisk->in_flight--;
> +
> +	disk_stat_add(mdev->vdisk, ticks[rw], duration);
> +}
> +
>  static void _req_is_done(drbd_dev *mdev, drbd_request_t *req, const int rw)
>  {
>  	const unsigned long s = req->rq_state;
> @@ -335,6 +366,9 @@ void _req_may_be_done(drbd_request_t *req, int error)
>  		 * then again, if it is a READ, it is not in the TL at all.
>  		 * is it still leagal to complete a READ during freeze? */
>  
> +		/* Update disk stats */
> +		_drbd_end_io_acct(mdev, req);
> +
>  		_complete_master_bio(mdev,req,
>  			  ok ? 0 : ( error ? error : -EIO ) );
>  	} else {
> @@ -956,6 +990,9 @@ drbd_make_request_common(drbd_dev *mdev, struct bio *bio)
>  	}
>  
>  
> +	/* Update disk stats */
> +	_drbd_start_io_acct(mdev, req, bio);
> +
>  	/* _maybe_start_new_epoch(mdev);
>  	 * If we need to generate a write barrier packet, we have to add the
>  	 * new epoch (barrier) object, and queue the barrier packet for sending,
> @@ -1012,6 +1049,7 @@ drbd_make_request_common(drbd_dev *mdev, struct bio *bio)
>  			local = 0;
>  		}
>  		if (remote) dec_ap_pending(mdev);
> +		_drbd_end_io_acct(mdev, req);
>  		/* THINK: do we want to fail it (-EIO), or pretend success? */
>  		bio_endio(req->master_bio, 0);
>  		req->master_bio = NULL;
> diff --git a/drbd/drbd_req.h b/drbd/drbd_req.h
> index 1d981b2..74d8dc1 100644
> --- a/drbd/drbd_req.h
> +++ b/drbd/drbd_req.h
> @@ -284,6 +284,7 @@ static inline drbd_request_t* drbd_req_new(drbd_dev *mdev, struct bio *bio_src)
>  		req->epoch       = 0;
>  		req->sector      = bio->bi_sector;
>  		req->size        = bio->bi_size;
> +		req->start_time  = jiffies;
>  		INIT_HLIST_NODE(&req->colision);
>  		INIT_LIST_HEAD(&req->tl_requests);
>  
> -- 
> 1.5.4-rc1.GIT
> 

> _______________________________________________
> drbd-dev mailing list
> drbd-dev@lists.linbit.com
> http://lists.linbit.com/mailman/listinfo/drbd-dev


-- 
: Lars Ellenberg                            Tel +43-1-8178292-55 :
: LINBIT Information Technologies GmbH      Fax +43-1-8178292-82 :
: Vivenotgasse 48, A-1120 Vienna/Europe    http://www.linbit.com :

RE: [Drbd-dev] [PATCH] - DRBD 8.0: Add standard disk counters toDRBD devices

[Graham, Simon]

> > +/* Update disk stats at start of I/O request */
> > +static inline void _drbd_start_io_acct(drbd_dev *mdev,
> drbd_request_t *req, struct bio *bio)
> > +{
> > +	int rw = bio_data_dir(bio);
> > +
> > +	MUST_HOLD(&mdev->req_lock)
> > +
> > +	disk_stat_inc(mdev->vdisk, ios[rw]);
> > +	disk_stat_add(mdev->vdisk, sectors[rw], bio_sectors(bio));
> > +	preempt_disable();
> 
> we are inside a spin_lock, no need to manipulate preempt,
> there is no preemptible spin_lock yet.
> 

I'll take your word for it BUT this is what ll_rw_blk.c/md do... well,
at least I think they do... I did merge some code in my version so maybe
they don't hold the lock when they call disk_round_stats()...

Simon