Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kernel API changes IO acct, global_page_state, etc #6635

Merged
merged 2 commits into from
Sep 16, 2017

Conversation

dinatale2
Copy link
Contributor

@dinatale2 dinatale2 commented Sep 12, 2017

Description

generic_start_io_acct/generic_end_io_acct in the master
branch of the linux kernel requires that the request_queue
be provided.

Move the logic from freemem in the spl to arc_free_memory
in arc.c. Do this so we can take advantage of global_page_state
interface checks in zfs.

Upstream kernel replaced struct block_device with
struct gendisk in struct bio. Determine if the
function bio_set_dev exists during configure
and have zfs use that if it exists.

Motivation and Context

Fix the Kernel builder in the ZFS buildbot.

How Has This Been Tested?

Builds in a VM locally. Going to see if buildbot builds work.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@dinatale2 dinatale2 force-pushed the generic_io_fixes branch 4 times, most recently from 32a5759 to 88f89f1 Compare September 12, 2017 21:16
@dinatale2 dinatale2 added the Status: Work in Progress Not yet ready for general review label Sep 12, 2017
@dinatale2 dinatale2 force-pushed the generic_io_fixes branch 7 times, most recently from a38d5e0 to 95a6122 Compare September 13, 2017 19:40
@dinatale2 dinatale2 changed the title Generic IO accounting accept request_queue Kernel API changes IO acct, global_page_state, etc Sep 13, 2017
@dinatale2 dinatale2 removed the Status: Work in Progress Not yet ready for general review label Sep 13, 2017
@chrisrd
Copy link
Contributor

chrisrd commented Sep 14, 2017

It's early days, but the global_page_state => global_node_page_state part of this fix, along with #6528, looks to have greatly stabilised my Linux 4.9 / 192GB system. Contrast the ARC, with the same workload, from Monday's boot prior to fix with previously-inexplicable wild gyrations in arcstats:c (Target Size in the graph) etc., to the current post-fix boot from Wed afternoon:

zfs_stats_yazol_arc_size-2

The summary of those fixes is that with the Linux API change introduced by torvalds/linux@599d0c9, our free memory calcs were based on completely the wrong thing. E.g. where we're looking at NR_FILE_PAGES, after the Linux change, and before these fixes, we were actually looking at one of NR_ZSPAGES, NUMA_HIT or NR_FREE_CMA_PAGES, depending on how your kernel is configured. (That was a particularly egregious API change in Linux - instead of breaking the compilation it silently did the wrong thing 👎 👎 👎)

This also goes a long way towards explaining various arc_prune issues (including #6630), with the arcstats:c collapses causing extreme efforts to reduce arcstats:size - particulary on larger memory boxes where a collapse of arcstats:c can cause the box to struggle to release many, many GB of memory.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally this looks good. Just a little suggested refactoring. I completely agree with @chrisrd , it's really unfortunate the kernel changed this behavior so significantly in a way which didn't break the build. I'm just glad it's been caught. @chrisrd your graph looks much more like what I'd expect, thank you for the quick feedback!

dr->dr_bio[i]->bi_bdev = bdev;
#else
bio_set_dev(dr->dr_bio[i], bdev);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be more readable if we defined our own version of bio_set_dev() in linux/blkdev_compat.h when !HAVE_BIO_SET_DEV. A similar thing was done for bio_set_op_attrs().

#ifndef HAVE_BIO_SET_DEV
static inline void
bio_set_dev(struct bio *bio, struct block_device *bdev)
{
        bio->bi_bdev = bdev;
}
#endif /* !HAVE_BIO_SET_DEV */

bio->bi_bdev = bdev;
#else
bio_set_dev(bio, bdev);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use bio_set_dev() which will now always be available.

dinatale2 and others added 2 commits September 15, 2017 19:17
generic_start_io_acct/generic_end_io_acct in the master
branch of the linux kernel requires that the request_queue
be provided.

Move the logic from freemem in the spl to arc_free_memory
in arc.c. Do this so we can take advantage of global_page_state
interface checks in zfs.

Upstream kernel replaced struct block_device with
struct gendisk in struct bio. Determine if the
function bio_set_dev exists during configure
and have zfs use that if it exists.

bio_set_dev torvalds/linux@74d4699
global_node_page_state torvalds/linux@75ef718
io acct torvalds/linux@d62e26b

Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf
Copy link
Contributor

@dinatale2 I added a commit to this PR to add the bio_set_dev() helper when needed.

@behlendorf behlendorf merged commit 787acae into openzfs:master Sep 16, 2017
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Sep 18, 2017
generic_start_io_acct/generic_end_io_acct in the master
branch of the linux kernel requires that the request_queue
be provided.

Move the logic from freemem in the spl to arc_free_memory
in arc.c. Do this so we can take advantage of global_page_state
interface checks in zfs.

Upstream kernel replaced struct block_device with
struct gendisk in struct bio. Determine if the
function bio_set_dev exists during configure
and have zfs use that if it exists.

bio_set_dev torvalds/linux@74d4699
global_node_page_state torvalds/linux@75ef718
io acct torvalds/linux@d62e26b

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#6635

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@dinatale2 dinatale2 deleted the generic_io_fixes branch September 19, 2017 20:04
tonyhutter pushed a commit that referenced this pull request Sep 19, 2017
generic_start_io_acct/generic_end_io_acct in the master
branch of the linux kernel requires that the request_queue
be provided.

Move the logic from freemem in the spl to arc_free_memory
in arc.c. Do this so we can take advantage of global_page_state
interface checks in zfs.

Upstream kernel replaced struct block_device with
struct gendisk in struct bio. Determine if the
function bio_set_dev exists during configure
and have zfs use that if it exists.

bio_set_dev torvalds/linux@74d4699
global_node_page_state torvalds/linux@75ef718
io acct torvalds/linux@d62e26b

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes #6635

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Fabian-Gruenbichler pushed a commit to Fabian-Gruenbichler/zfs that referenced this pull request Sep 29, 2017
generic_start_io_acct/generic_end_io_acct in the master
branch of the linux kernel requires that the request_queue
be provided.

Move the logic from freemem in the spl to arc_free_memory
in arc.c. Do this so we can take advantage of global_page_state
interface checks in zfs.

Upstream kernel replaced struct block_device with
struct gendisk in struct bio. Determine if the
function bio_set_dev exists during configure
and have zfs use that if it exists.

bio_set_dev torvalds/linux@74d4699
global_node_page_state torvalds/linux@75ef718
io acct torvalds/linux@d62e26b

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#6635

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants