The "force" parameter in __blk_queue_bounce was being ignored, which means that
stable page snapshots are not always happening (on ext3). This of course
leads to DIF disks reporting checksum errors, so fix this regression.
The regression was introduced in commit 6bc454d1 (bounce: Refactor
__blk_queue_bounce to not use bi_io_vec)
Reported-by: Mel Gorman <[email protected]>
Signed-off-by: Darrick J. Wong <[email protected]>
---
mm/bounce.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/bounce.c b/mm/bounce.c
index c9f0a43..5a7d58f 100644
--- a/mm/bounce.c
+++ b/mm/bounce.c
@@ -204,6 +204,8 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
struct bio_vec *to, *from;
unsigned i;
+ if (force)
+ goto bounce;
bio_for_each_segment(from, *bio_orig, i)
if (page_to_pfn(from->bv_page) > queue_bounce_pfn(q))
goto bounce;
On Thu, Sep 19, 2013 at 11:06 PM, Darrick J. Wong
<[email protected]> wrote:
> The "force" parameter in __blk_queue_bounce was being ignored, which means that
> stable page snapshots are not always happening (on ext3). This of course
> leads to DIF disks reporting checksum errors, so fix this regression.
>
> The regression was introduced in commit 6bc454d1 (bounce: Refactor
> __blk_queue_bounce to not use bi_io_vec)
Should probably add "Cc: [email protected] # v3.10+" no?
On Thu, Sep 19, 2013 at 08:06:02PM -0700, Darrick J. Wong wrote:
> The "force" parameter in __blk_queue_bounce was being ignored, which means that
> stable page snapshots are not always happening (on ext3). This of course
> leads to DIF disks reporting checksum errors, so fix this regression.
>
> The regression was introduced in commit 6bc454d1 (bounce: Refactor
> __blk_queue_bounce to not use bi_io_vec)
>
> Reported-by: Mel Gorman <[email protected]>
> Signed-off-by: Darrick J. Wong <[email protected]>
I have no means of testing it but it looks right and thanks for checking
DIF disks.
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
On Fri, Sep 20, 2013 at 09:06:37AM +0100, Mel Gorman wrote:
> On Thu, Sep 19, 2013 at 08:06:02PM -0700, Darrick J. Wong wrote:
> > The "force" parameter in __blk_queue_bounce was being ignored, which means that
> > stable page snapshots are not always happening (on ext3). This of course
> > leads to DIF disks reporting checksum errors, so fix this regression.
> >
> > The regression was introduced in commit 6bc454d1 (bounce: Refactor
> > __blk_queue_bounce to not use bi_io_vec)
> >
> > Reported-by: Mel Gorman <[email protected]>
> > Signed-off-by: Darrick J. Wong <[email protected]>
>
> I have no means of testing it but it looks right and thanks for checking
> DIF disks.
>
> Acked-by: Mel Gorman <[email protected]>
>
That said the two checks are now redundant. They could just be deleted
and depend entirely on the following check within the loop
if (page_to_pfn(page) <= queue_bounce_pfn(q) && !force)
continue;
with an update to the comment explaining that the check is for pages
below the bounce pfn or for bios that require stable writes
--
Mel Gorman
SUSE Labs
On Fri, Sep 20, 2013 at 09:28:07AM +0100, Mel Gorman wrote:
> On Fri, Sep 20, 2013 at 09:06:37AM +0100, Mel Gorman wrote:
> > On Thu, Sep 19, 2013 at 08:06:02PM -0700, Darrick J. Wong wrote:
> > > The "force" parameter in __blk_queue_bounce was being ignored, which means that
> > > stable page snapshots are not always happening (on ext3). This of course
> > > leads to DIF disks reporting checksum errors, so fix this regression.
> > >
> > > The regression was introduced in commit 6bc454d1 (bounce: Refactor
> > > __blk_queue_bounce to not use bi_io_vec)
> > >
> > > Reported-by: Mel Gorman <[email protected]>
> > > Signed-off-by: Darrick J. Wong <[email protected]>
> >
> > I have no means of testing it but it looks right and thanks for checking
> > DIF disks.
> >
> > Acked-by: Mel Gorman <[email protected]>
> >
>
> That said the two checks are now redundant. They could just be deleted
> and depend entirely on the following check within the loop
>
> if (page_to_pfn(page) <= queue_bounce_pfn(q) && !force)
> continue;
>
> with an update to the comment explaining that the check is for pages
> below the bounce pfn or for bios that require stable writes
I'm under the impression that the point of the first looping-check is to avoid
the bio_clone_bioset() if we're not going to bounce anything. But I could be
wrong. Maybe Kent can shed some light on this?
--D
>
> --
> Mel Gorman
> SUSE Labs
On Fri, Sep 20, 2013 at 09:54:07AM -0700, Darrick J. Wong wrote:
> On Fri, Sep 20, 2013 at 09:28:07AM +0100, Mel Gorman wrote:
> > On Fri, Sep 20, 2013 at 09:06:37AM +0100, Mel Gorman wrote:
> > > On Thu, Sep 19, 2013 at 08:06:02PM -0700, Darrick J. Wong wrote:
> > > > The "force" parameter in __blk_queue_bounce was being ignored, which means that
> > > > stable page snapshots are not always happening (on ext3). This of course
> > > > leads to DIF disks reporting checksum errors, so fix this regression.
> > > >
> > > > The regression was introduced in commit 6bc454d1 (bounce: Refactor
> > > > __blk_queue_bounce to not use bi_io_vec)
> > > >
> > > > Reported-by: Mel Gorman <[email protected]>
> > > > Signed-off-by: Darrick J. Wong <[email protected]>
> > >
> > > I have no means of testing it but it looks right and thanks for checking
> > > DIF disks.
> > >
> > > Acked-by: Mel Gorman <[email protected]>
> > >
> >
> > That said the two checks are now redundant. They could just be deleted
> > and depend entirely on the following check within the loop
> >
> > if (page_to_pfn(page) <= queue_bounce_pfn(q) && !force)
> > continue;
> >
> > with an update to the comment explaining that the check is for pages
> > below the bounce pfn or for bios that require stable writes
>
> I'm under the impression that the point of the first looping-check is to avoid
> the bio_clone_bioset() if we're not going to bounce anything. But I could be
> wrong.
I doubt it, avoiding the clone makes sense and I was wrong about the
check being redundant anyway. Trying to do the clone within the loop
when the first bio that needs bouncing is encountered turns into a
mess.
--
Mel Gorman
SUSE Labs