On Fri, 4 Nov 2022, NeilBrown wrote:
> > ---
> > drivers/md/md.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6/drivers/md/md.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/md/md.c 2022-11-03 15:29:02.000000000 +0100
> > +++ linux-2.6/drivers/md/md.c 2022-11-03 15:33:17.000000000 +0100
> > @@ -509,13 +509,14 @@ static void md_end_flush(struct bio *bio
> > struct md_rdev *rdev = bio->bi_private;
> > struct mddev *mddev = rdev->mddev;
> >
> > + bio_put(bio);
> > +
> > rdev_dec_pending(rdev, mddev);
> >
> > if (atomic_dec_and_test(&mddev->flush_pending)) {
> > /* The pre-request flush has finished */
> > queue_work(md_wq, &mddev->flush_work);
> > }
> > - bio_put(bio);
> > }
> >
> > static void md_submit_flush_data(struct work_struct *ws);
> > @@ -913,10 +914,11 @@ static void super_written(struct bio *bi
> > } else
> > clear_bit(LastDev, &rdev->flags);
> >
> > + bio_put(bio);
> > +
> > if (atomic_dec_and_test(&mddev->pending_writes))
> > wake_up(&mddev->sb_wait);
> > rdev_dec_pending(rdev, mddev);
> > - bio_put(bio);
> > }
>
> Thanks. I think this is a clear improvement.
> I think it would be a little better if the rdev_dec_pending were also
> move up.
> Then both code fragments would be:
> bio_put ; rdev_dec_pending ; atomic_dec_and_test
>
> Thanks,
> NeilBrown
Yes, I'll send a second patch that moves rdev_dec_pending up too.
BTW. even this is theoretically incorrect:
> > if (atomic_dec_and_test(&mddev->pending_writes))
> > wake_up(&mddev->sb_wait);
Suppose that you execute atomic_dec_and_test and then there's a context
switch to a process that destroys the md device and then there's a context
switch back and you call "wake_up(&mddev->sb_wait)" on freed memory.
I think that we should use wait_var_event/wake_up_var instead of sb_wait.
That will use preallocated hashed wait queues.
Mikulas
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
