From: Guoqing Jiang <[email protected]>
commit ad3fc798800fb7ca04c1dfc439dba946818048d8 upstream.
The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
double fault problem per the report [1], and also it is not correct to
change ->bi_end_io if md don't own it, so let's revert it.
And io stats accounting will be replemented in later commits.
[1]. https://lore.kernel.org/linux-raid/[email protected]/T/#t
Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
Signed-off-by: Guoqing Jiang <[email protected]>
Signed-off-by: Song Liu <[email protected]>
[GM: backport to 5.10-stable]
Signed-off-by: Guillaume Morin <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/md/md.c | 57 +++++++++++---------------------------------------------
drivers/md/md.h | 1
2 files changed, 12 insertions(+), 46 deletions(-)
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -459,34 +459,12 @@ check_suspended:
}
EXPORT_SYMBOL(md_handle_request);
-struct md_io {
- struct mddev *mddev;
- bio_end_io_t *orig_bi_end_io;
- void *orig_bi_private;
- unsigned long start_time;
- struct hd_struct *part;
-};
-
-static void md_end_io(struct bio *bio)
-{
- struct md_io *md_io = bio->bi_private;
- struct mddev *mddev = md_io->mddev;
-
- part_end_io_acct(md_io->part, bio, md_io->start_time);
-
- bio->bi_end_io = md_io->orig_bi_end_io;
- bio->bi_private = md_io->orig_bi_private;
-
- mempool_free(md_io, &mddev->md_io_pool);
-
- if (bio->bi_end_io)
- bio->bi_end_io(bio);
-}
-
static blk_qc_t md_submit_bio(struct bio *bio)
{
const int rw = bio_data_dir(bio);
+ const int sgrp = op_stat_group(bio_op(bio));
struct mddev *mddev = bio->bi_disk->private_data;
+ unsigned int sectors;
if (mddev == NULL || mddev->pers == NULL) {
bio_io_error(bio);
@@ -507,26 +485,21 @@ static blk_qc_t md_submit_bio(struct bio
return BLK_QC_T_NONE;
}
- if (bio->bi_end_io != md_end_io) {
- struct md_io *md_io;
-
- md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
- md_io->mddev = mddev;
- md_io->orig_bi_end_io = bio->bi_end_io;
- md_io->orig_bi_private = bio->bi_private;
-
- bio->bi_end_io = md_end_io;
- bio->bi_private = md_io;
-
- md_io->start_time = part_start_io_acct(mddev->gendisk,
- &md_io->part, bio);
- }
-
+ /*
+ * save the sectors now since our bio can
+ * go away inside make_request
+ */
+ sectors = bio_sectors(bio);
/* bio could be mergeable after passing to underlayer */
bio->bi_opf &= ~REQ_NOMERGE;
md_handle_request(mddev, bio);
+ part_stat_lock();
+ part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
+ part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
+ part_stat_unlock();
+
return BLK_QC_T_NONE;
}
@@ -5636,7 +5609,6 @@ static void md_free(struct kobject *ko)
bioset_exit(&mddev->bio_set);
bioset_exit(&mddev->sync_set);
- mempool_exit(&mddev->md_io_pool);
kfree(mddev);
}
@@ -5732,11 +5704,6 @@ static int md_alloc(dev_t dev, char *nam
*/
mddev->hold_active = UNTIL_STOP;
- error = mempool_init_kmalloc_pool(&mddev->md_io_pool, BIO_POOL_SIZE,
- sizeof(struct md_io));
- if (error)
- goto abort;
-
error = -ENOMEM;
mddev->queue = blk_alloc_queue(NUMA_NO_NODE);
if (!mddev->queue)
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -487,7 +487,6 @@ struct mddev {
struct bio_set sync_set; /* for sync operations like
* metadata and bitmap writes
*/
- mempool_t md_io_pool;
/* Generic flush handling.
* The last to finish preflush schedules a worker to submit
Hi,
Greg Kroah-Hartman <[email protected]> 于2022年1月14日周五 19:57写道：
>
> From: Guoqing Jiang <[email protected]>
>
> commit ad3fc798800fb7ca04c1dfc439dba946818048d8 upstream.
>
> The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
> double fault problem per the report [1], and also it is not correct to
> change ->bi_end_io if md don't own it, so let's revert it.
>
> And io stats accounting will be replemented in later commits.
>
> [1]. https://lore.kernel.org/linux-raid/[email protected]/T/#t
>
> Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
> Signed-off-by: Guoqing Jiang <[email protected]>
> Signed-off-by: Song Liu <[email protected]>
> [GM: backport to 5.10-stable]
> Signed-off-by: Guillaume Morin <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/md/md.c | 57 +++++++++++---------------------------------------------
> drivers/md/md.h | 1
> 2 files changed, 12 insertions(+), 46 deletions(-)
>
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -459,34 +459,12 @@ check_suspended:
> }
> EXPORT_SYMBOL(md_handle_request);
>
> -struct md_io {
> - struct mddev *mddev;
> - bio_end_io_t *orig_bi_end_io;
> - void *orig_bi_private;
> - unsigned long start_time;
> - struct hd_struct *part;
> -};
> -
> -static void md_end_io(struct bio *bio)
> -{
> - struct md_io *md_io = bio->bi_private;
> - struct mddev *mddev = md_io->mddev;
> -
> - part_end_io_acct(md_io->part, bio, md_io->start_time);
> -
> - bio->bi_end_io = md_io->orig_bi_end_io;
> - bio->bi_private = md_io->orig_bi_private;
> -
> - mempool_free(md_io, &mddev->md_io_pool);
> -
> - if (bio->bi_end_io)
> - bio->bi_end_io(bio);
> -}
> -
> static blk_qc_t md_submit_bio(struct bio *bio)
> {
> const int rw = bio_data_dir(bio);
> + const int sgrp = op_stat_group(bio_op(bio));
> struct mddev *mddev = bio->bi_disk->private_data;
> + unsigned int sectors;
>
> if (mddev == NULL || mddev->pers == NULL) {
> bio_io_error(bio);
> @@ -507,26 +485,21 @@ static blk_qc_t md_submit_bio(struct bio
> return BLK_QC_T_NONE;
> }
>
> - if (bio->bi_end_io != md_end_io) {
> - struct md_io *md_io;
> -
> - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> - md_io->mddev = mddev;
> - md_io->orig_bi_end_io = bio->bi_end_io;
> - md_io->orig_bi_private = bio->bi_private;
> -
> - bio->bi_end_io = md_end_io;
> - bio->bi_private = md_io;
> -
> - md_io->start_time = part_start_io_acct(mddev->gendisk,
> - &md_io->part, bio);
> - }
> -
> + /*
> + * save the sectors now since our bio can
> + * go away inside make_request
> + */
> + sectors = bio_sectors(bio);
This code snip is not inside the original patch, and it's not in
latest upstream too.
> /* bio could be mergeable after passing to underlayer */
> bio->bi_opf &= ~REQ_NOMERGE;
>
> md_handle_request(mddev, bio);
>
> + part_stat_lock();
> + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> + part_stat_unlock();
> +
same here, this code snip is not inside the original patch, and it's
not in latest upstream too.
I think would be good keep it as the upstream version.
Best!
Jinpu Wang @ IONOS
> return BLK_QC_T_NONE;
> }
>
> @@ -5636,7 +5609,6 @@ static void md_free(struct kobject *ko)
>
> bioset_exit(&mddev->bio_set);
> bioset_exit(&mddev->sync_set);
> - mempool_exit(&mddev->md_io_pool);
> kfree(mddev);
> }
>
> @@ -5732,11 +5704,6 @@ static int md_alloc(dev_t dev, char *nam
> */
> mddev->hold_active = UNTIL_STOP;
>
> - error = mempool_init_kmalloc_pool(&mddev->md_io_pool, BIO_POOL_SIZE,
> - sizeof(struct md_io));
> - if (error)
> - goto abort;
> -
> error = -ENOMEM;
> mddev->queue = blk_alloc_queue(NUMA_NO_NODE);
> if (!mddev->queue)
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -487,7 +487,6 @@ struct mddev {
> struct bio_set sync_set; /* for sync operations like
> * metadata and bitmap writes
> */
> - mempool_t md_io_pool;
>
> /* Generic flush handling.
> * The last to finish preflush schedules a worker to submit
>
>
On Wed, Jan 26, 2022 at 11:09:46AM +0100, Jack Wang wrote:
> Hi,
>
> Greg Kroah-Hartman <[email protected]> 于2022年1月14日周五 19:57写道：
> >
> > From: Guoqing Jiang <[email protected]>
> >
> > commit ad3fc798800fb7ca04c1dfc439dba946818048d8 upstream.
> >
> > The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
> > double fault problem per the report [1], and also it is not correct to
> > change ->bi_end_io if md don't own it, so let's revert it.
> >
> > And io stats accounting will be replemented in later commits.
> >
> > [1]. https://lore.kernel.org/linux-raid/[email protected]/T/#t
> >
> > Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
> > Signed-off-by: Guoqing Jiang <[email protected]>
> > Signed-off-by: Song Liu <[email protected]>
> > [GM: backport to 5.10-stable]
> > Signed-off-by: Guillaume Morin <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > drivers/md/md.c | 57 +++++++++++---------------------------------------------
> > drivers/md/md.h | 1
> > 2 files changed, 12 insertions(+), 46 deletions(-)
> >
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -459,34 +459,12 @@ check_suspended:
> > }
> > EXPORT_SYMBOL(md_handle_request);
> >
> > -struct md_io {
> > - struct mddev *mddev;
> > - bio_end_io_t *orig_bi_end_io;
> > - void *orig_bi_private;
> > - unsigned long start_time;
> > - struct hd_struct *part;
> > -};
> > -
> > -static void md_end_io(struct bio *bio)
> > -{
> > - struct md_io *md_io = bio->bi_private;
> > - struct mddev *mddev = md_io->mddev;
> > -
> > - part_end_io_acct(md_io->part, bio, md_io->start_time);
> > -
> > - bio->bi_end_io = md_io->orig_bi_end_io;
> > - bio->bi_private = md_io->orig_bi_private;
> > -
> > - mempool_free(md_io, &mddev->md_io_pool);
> > -
> > - if (bio->bi_end_io)
> > - bio->bi_end_io(bio);
> > -}
> > -
> > static blk_qc_t md_submit_bio(struct bio *bio)
> > {
> > const int rw = bio_data_dir(bio);
> > + const int sgrp = op_stat_group(bio_op(bio));
> > struct mddev *mddev = bio->bi_disk->private_data;
> > + unsigned int sectors;
> >
> > if (mddev == NULL || mddev->pers == NULL) {
> > bio_io_error(bio);
> > @@ -507,26 +485,21 @@ static blk_qc_t md_submit_bio(struct bio
> > return BLK_QC_T_NONE;
> > }
> >
> > - if (bio->bi_end_io != md_end_io) {
> > - struct md_io *md_io;
> > -
> > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > - md_io->mddev = mddev;
> > - md_io->orig_bi_end_io = bio->bi_end_io;
> > - md_io->orig_bi_private = bio->bi_private;
> > -
> > - bio->bi_end_io = md_end_io;
> > - bio->bi_private = md_io;
> > -
> > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > - &md_io->part, bio);
> > - }
> > -
> > + /*
> > + * save the sectors now since our bio can
> > + * go away inside make_request
> > + */
> > + sectors = bio_sectors(bio);
> This code snip is not inside the original patch, and it's not in
> latest upstream too.
> > /* bio could be mergeable after passing to underlayer */
> > bio->bi_opf &= ~REQ_NOMERGE;
> >
> > md_handle_request(mddev, bio);
> >
> > + part_stat_lock();
> > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > + part_stat_unlock();
> > +
> same here, this code snip is not inside the original patch, and it's
> not in latest upstream too.
Is it a problem?
> I think would be good keep it as the upstream version.
Can you send a revert of this commit (it is in 5.10.92), and a backport
of the correct fix?
thanks,
greg k-h
Greg Kroah-Hartman <[email protected]> 于2022年1月26日周三 12:42写道：
>
> On Wed, Jan 26, 2022 at 11:09:46AM +0100, Jack Wang wrote:
> > Hi,
> >
> > Greg Kroah-Hartman <[email protected]> 于2022年1月14日周五 19:57写道：
> > >
> > > From: Guoqing Jiang <[email protected]>
> > >
> > > commit ad3fc798800fb7ca04c1dfc439dba946818048d8 upstream.
> > >
> > > The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
> > > double fault problem per the report [1], and also it is not correct to
> > > change ->bi_end_io if md don't own it, so let's revert it.
> > >
> > > And io stats accounting will be replemented in later commits.
> > >
> > > [1]. https://lore.kernel.org/linux-raid/[email protected]/T/#t
> > >
> > > Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
> > > Signed-off-by: Guoqing Jiang <[email protected]>
> > > Signed-off-by: Song Liu <[email protected]>
> > > [GM: backport to 5.10-stable]
> > > Signed-off-by: Guillaume Morin <[email protected]>
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > ---
> > > drivers/md/md.c | 57 +++++++++++---------------------------------------------
> > > drivers/md/md.h | 1
> > > 2 files changed, 12 insertions(+), 46 deletions(-)
> > >
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -459,34 +459,12 @@ check_suspended:
> > > }
> > > EXPORT_SYMBOL(md_handle_request);
> > >
> > > -struct md_io {
> > > - struct mddev *mddev;
> > > - bio_end_io_t *orig_bi_end_io;
> > > - void *orig_bi_private;
> > > - unsigned long start_time;
> > > - struct hd_struct *part;
> > > -};
> > > -
> > > -static void md_end_io(struct bio *bio)
> > > -{
> > > - struct md_io *md_io = bio->bi_private;
> > > - struct mddev *mddev = md_io->mddev;
> > > -
> > > - part_end_io_acct(md_io->part, bio, md_io->start_time);
> > > -
> > > - bio->bi_end_io = md_io->orig_bi_end_io;
> > > - bio->bi_private = md_io->orig_bi_private;
> > > -
> > > - mempool_free(md_io, &mddev->md_io_pool);
> > > -
> > > - if (bio->bi_end_io)
> > > - bio->bi_end_io(bio);
> > > -}
> > > -
> > > static blk_qc_t md_submit_bio(struct bio *bio)
> > > {
> > > const int rw = bio_data_dir(bio);
> > > + const int sgrp = op_stat_group(bio_op(bio));
> > > struct mddev *mddev = bio->bi_disk->private_data;
> > > + unsigned int sectors;
> > >
> > > if (mddev == NULL || mddev->pers == NULL) {
> > > bio_io_error(bio);
> > > @@ -507,26 +485,21 @@ static blk_qc_t md_submit_bio(struct bio
> > > return BLK_QC_T_NONE;
> > > }
> > >
> > > - if (bio->bi_end_io != md_end_io) {
> > > - struct md_io *md_io;
> > > -
> > > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > > - md_io->mddev = mddev;
> > > - md_io->orig_bi_end_io = bio->bi_end_io;
> > > - md_io->orig_bi_private = bio->bi_private;
> > > -
> > > - bio->bi_end_io = md_end_io;
> > > - bio->bi_private = md_io;
> > > -
> > > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > > - &md_io->part, bio);
> > > - }
> > > -
> > > + /*
> > > + * save the sectors now since our bio can
> > > + * go away inside make_request
> > > + */
> > > + sectors = bio_sectors(bio);
> > This code snip is not inside the original patch, and it's not in
> > latest upstream too.
> > > /* bio could be mergeable after passing to underlayer */
> > > bio->bi_opf &= ~REQ_NOMERGE;
> > >
> > > md_handle_request(mddev, bio);
> > >
> > > + part_stat_lock();
> > > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > > + part_stat_unlock();
> > > +
> > same here, this code snip is not inside the original patch, and it's
> > not in latest upstream too.
>
> Is it a problem?
Not sure, might cause some confusion regarding io stats.
>
> > I think would be good keep it as the upstream version.
>
> Can you send a revert of this commit (it is in 5.10.92), and a backport
> of the correct fix?
>
sure, I just sent an incremental fix for the backport itself.
is it ok?
Thanks!
> thanks,
>
> greg k-h
On Wed, Jan 26, 2022 at 01:37:12PM +0100, Jack Wang wrote:
> Greg Kroah-Hartman <[email protected]> 于2022年1月26日周三 12:42写道：
> >
> > On Wed, Jan 26, 2022 at 11:09:46AM +0100, Jack Wang wrote:
> > > Hi,
> > >
> > > Greg Kroah-Hartman <[email protected]> 于2022年1月14日周五 19:57写道：
> > > >
> > > > From: Guoqing Jiang <[email protected]>
> > > >
> > > > commit ad3fc798800fb7ca04c1dfc439dba946818048d8 upstream.
> > > >
> > > > The commit 41d2d848e5c0 ("md: improve io stats accounting") could cause
> > > > double fault problem per the report [1], and also it is not correct to
> > > > change ->bi_end_io if md don't own it, so let's revert it.
> > > >
> > > > And io stats accounting will be replemented in later commits.
> > > >
> > > > [1]. https://lore.kernel.org/linux-raid/[email protected]/T/#t
> > > >
> > > > Fixes: 41d2d848e5c0 ("md: improve io stats accounting")
> > > > Signed-off-by: Guoqing Jiang <[email protected]>
> > > > Signed-off-by: Song Liu <[email protected]>
> > > > [GM: backport to 5.10-stable]
> > > > Signed-off-by: Guillaume Morin <[email protected]>
> > > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > > > ---
> > > > drivers/md/md.c | 57 +++++++++++---------------------------------------------
> > > > drivers/md/md.h | 1
> > > > 2 files changed, 12 insertions(+), 46 deletions(-)
> > > >
> > > > --- a/drivers/md/md.c
> > > > +++ b/drivers/md/md.c
> > > > @@ -459,34 +459,12 @@ check_suspended:
> > > > }
> > > > EXPORT_SYMBOL(md_handle_request);
> > > >
> > > > -struct md_io {
> > > > - struct mddev *mddev;
> > > > - bio_end_io_t *orig_bi_end_io;
> > > > - void *orig_bi_private;
> > > > - unsigned long start_time;
> > > > - struct hd_struct *part;
> > > > -};
> > > > -
> > > > -static void md_end_io(struct bio *bio)
> > > > -{
> > > > - struct md_io *md_io = bio->bi_private;
> > > > - struct mddev *mddev = md_io->mddev;
> > > > -
> > > > - part_end_io_acct(md_io->part, bio, md_io->start_time);
> > > > -
> > > > - bio->bi_end_io = md_io->orig_bi_end_io;
> > > > - bio->bi_private = md_io->orig_bi_private;
> > > > -
> > > > - mempool_free(md_io, &mddev->md_io_pool);
> > > > -
> > > > - if (bio->bi_end_io)
> > > > - bio->bi_end_io(bio);
> > > > -}
> > > > -
> > > > static blk_qc_t md_submit_bio(struct bio *bio)
> > > > {
> > > > const int rw = bio_data_dir(bio);
> > > > + const int sgrp = op_stat_group(bio_op(bio));
> > > > struct mddev *mddev = bio->bi_disk->private_data;
> > > > + unsigned int sectors;
> > > >
> > > > if (mddev == NULL || mddev->pers == NULL) {
> > > > bio_io_error(bio);
> > > > @@ -507,26 +485,21 @@ static blk_qc_t md_submit_bio(struct bio
> > > > return BLK_QC_T_NONE;
> > > > }
> > > >
> > > > - if (bio->bi_end_io != md_end_io) {
> > > > - struct md_io *md_io;
> > > > -
> > > > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > > > - md_io->mddev = mddev;
> > > > - md_io->orig_bi_end_io = bio->bi_end_io;
> > > > - md_io->orig_bi_private = bio->bi_private;
> > > > -
> > > > - bio->bi_end_io = md_end_io;
> > > > - bio->bi_private = md_io;
> > > > -
> > > > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > > > - &md_io->part, bio);
> > > > - }
> > > > -
> > > > + /*
> > > > + * save the sectors now since our bio can
> > > > + * go away inside make_request
> > > > + */
> > > > + sectors = bio_sectors(bio);
> > > This code snip is not inside the original patch, and it's not in
> > > latest upstream too.
> > > > /* bio could be mergeable after passing to underlayer */
> > > > bio->bi_opf &= ~REQ_NOMERGE;
> > > >
> > > > md_handle_request(mddev, bio);
> > > >
> > > > + part_stat_lock();
> > > > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > > > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > > > + part_stat_unlock();
> > > > +
> > > same here, this code snip is not inside the original patch, and it's
> > > not in latest upstream too.
> >
> > Is it a problem?
> Not sure, might cause some confusion regarding io stats.
> >
> > > I think would be good keep it as the upstream version.
> >
> > Can you send a revert of this commit (it is in 5.10.92), and a backport
> > of the correct fix?
> >
> sure, I just sent an incremental fix for the backport itself.
> is it ok?
That works, I'll queue it up after this next round of releases, thanks!
greg k-h
On 26 Jan 11:09, Jack Wang wrote:
> >
> > - if (bio->bi_end_io != md_end_io) {
> > - struct md_io *md_io;
> > -
> > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > - md_io->mddev = mddev;
> > - md_io->orig_bi_end_io = bio->bi_end_io;
> > - md_io->orig_bi_private = bio->bi_private;
> > -
> > - bio->bi_end_io = md_end_io;
> > - bio->bi_private = md_io;
> > -
> > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > - &md_io->part, bio);
> > - }
> > -
> > + /*
> > + * save the sectors now since our bio can
> > + * go away inside make_request
> > + */
> > + sectors = bio_sectors(bio);
> This code snip is not inside the original patch, and it's not in
> latest upstream too.
> > /* bio could be mergeable after passing to underlayer */
> > bio->bi_opf &= ~REQ_NOMERGE;
> >
> > md_handle_request(mddev, bio);
> >
> > + part_stat_lock();
> > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > + part_stat_unlock();
> > +
> same here, this code snip is not inside the original patch, and it's
> not in latest upstream too.
Both snippets came from the code before 41d2d848e5c0 that the patch is
being reverted here. As I explained in my original message, upstream is
different because of 99dfc43ecbf6 which is not in 5.10.
> I think would be good keep it as the upstream version.
If you don't include these lines, isn't this worse as it's not calling
either part_start_io_acct or bio_start_io_acct (in 99dfc43ecbf6)?
--
Guillaume Morin <[email protected]>
On 26 Jan 13:37, Jack Wang wrote:
> > > >
> > > > - if (bio->bi_end_io != md_end_io) {
> > > > - struct md_io *md_io;
> > > > -
> > > > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > > > - md_io->mddev = mddev;
> > > > - md_io->orig_bi_end_io = bio->bi_end_io;
> > > > - md_io->orig_bi_private = bio->bi_private;
> > > > -
> > > > - bio->bi_end_io = md_end_io;
> > > > - bio->bi_private = md_io;
> > > > -
> > > > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > > > - &md_io->part, bio);
> > > > - }
> > > > -
> > > > + /*
> > > > + * save the sectors now since our bio can
> > > > + * go away inside make_request
> > > > + */
> > > > + sectors = bio_sectors(bio);
> > > This code snip is not inside the original patch, and it's not in
> > > latest upstream too.
> > > > /* bio could be mergeable after passing to underlayer */
> > > > bio->bi_opf &= ~REQ_NOMERGE;
> > > >
> > > > md_handle_request(mddev, bio);
> > > >
> > > > + part_stat_lock();
> > > > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > > > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > > > + part_stat_unlock();
> > > > +
> > > same here, this code snip is not inside the original patch, and it's
> > > not in latest upstream too.
> >
> > Is it a problem?
> Not sure, might cause some confusion regarding io stats.
Have you observed anything weird? Because if you don't include this, I
don't see where these stats are updated at all. Could you explain
please?
--
Guillaume Morin <[email protected]>
Guillaume Morin <[email protected]> 于2022年1月26日周三 16:12写道：
>
> On 26 Jan 11:09, Jack Wang wrote:
> > >
> > > - if (bio->bi_end_io != md_end_io) {
> > > - struct md_io *md_io;
> > > -
> > > - md_io = mempool_alloc(&mddev->md_io_pool, GFP_NOIO);
> > > - md_io->mddev = mddev;
> > > - md_io->orig_bi_end_io = bio->bi_end_io;
> > > - md_io->orig_bi_private = bio->bi_private;
> > > -
> > > - bio->bi_end_io = md_end_io;
> > > - bio->bi_private = md_io;
> > > -
> > > - md_io->start_time = part_start_io_acct(mddev->gendisk,
> > > - &md_io->part, bio);
> > > - }
> > > -
> > > + /*
> > > + * save the sectors now since our bio can
> > > + * go away inside make_request
> > > + */
> > > + sectors = bio_sectors(bio);
> > This code snip is not inside the original patch, and it's not in
> > latest upstream too.
> > > /* bio could be mergeable after passing to underlayer */
> > > bio->bi_opf &= ~REQ_NOMERGE;
> > >
> > > md_handle_request(mddev, bio);
> > >
> > > + part_stat_lock();
> > > + part_stat_inc(&mddev->gendisk->part0, ios[sgrp]);
> > > + part_stat_add(&mddev->gendisk->part0, sectors[sgrp], sectors);
> > > + part_stat_unlock();
> > > +
> > same here, this code snip is not inside the original patch, and it's
> > not in latest upstream too.
>
> Both snippets came from the code before 41d2d848e5c0 that the patch is
> being reverted here. As I explained in my original message, upstream is
> different because of 99dfc43ecbf6 which is not in 5.10.
oh, I missed it, you are right.
>
> > I think would be good keep it as the upstream version.
>
> If you don't include these lines, isn't this worse as it's not calling
> either part_start_io_acct or bio_start_io_acct (in 99dfc43ecbf6)?
Your patch is correct.
Sorry for the noise.
>
> --
> Guillaume Morin <[email protected]>