lkml.org 
[lkml]   [2009]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] swap: Fix swap size in case of block devices
    On Mon, 31 Aug 2009, Nitin Gupta wrote:
    >
    > mkswap sets last_page correctly: 0-based index of last usable
    > swap page. To explain why this bug affects only block swap devices,
    > some code walkthrough is done below:
    > (BTW, I only checked mkswap which is part of util-linux-ng 2.14.2).
    >
    > swapon()
    > {
    > ...
    > nr_good_pages = swap_header->info.last_page -
    > swap_header->info.nr_badpages -
    > 1 /* header page */;
    >
    > ====
    > off-by-one error: for both regular and block device case, but...
    > ====
    >
    > if (nr_good_pages) {
    > swap_map[0] = SWAP_MAP_BAD;
    > p->max = maxpages;
    > p->pages = nr_good_pages;
    > nr_extents = setup_swap_extents(p, &span);
    > ====
    > For block devices, setup_swap_extents() leaves p->pages untouched.
    > For regular files, it sets p->pages
    > == total usable swap pages (including header page) - 1;

    I think you're overlooking the "page < sis->max" condition
    in setup_swap_extents()'s loop. So at the end of the loop,
    if no pages were lost to fragmentation, we have

    sis->max = page_no; /* no change */
    sis->pages = page_no - 1; /* no change */

    > ====
    > if (nr_extents < 0) {
    > error = nr_extents;
    > goto bad_swap;
    > }
    > nr_good_pages = p->pages;
    >
    > ====
    > So, for block device, nr_good_pages == last_page - nr_badpages - 1
    > == (total pages - 1) - nr_badpages - 1 (error)
    > For regular files, nr_good_pages == total pages - 1
    > (correct)
    > ====
    >
    > }
    > ...
    > }
    >
    >
    > With this fix, block device case is corrected to last_page - nr_badpages - 1
    > while regular file case remain correct since setup_swap_extents() still gives
    > same correct value in p->pages (== total pages - 1).
    >
    >
    > > And regarding the patch itself: my understanding is that the problem
    > > is with the interpretation of last_page, so I don't think one change
    > > to nr_good_pages would be enough to fix it - you'd need to change the
    > > other places where last_page is referred to too.
    > >
    >
    > I looked at other instances of last_page in swapon() -- all these other
    > instances looked correct to me.

    I believe they're all consistent with the off-by-oneness of nr_good_pages.
    p->max, for example, is consistently one more than p->pages, so long as
    there are no bad pages and no overflowing the swp_entry_t.

    Perhaps you're placing too much faith in your interpretation of "max"?
    I dislike several conventions in swapfile.c, it does lend itself to
    off-by-oneness.

    >
    > > I'm still disinclined to make any change here myself (beyond
    > > a comment noting the discrepancy); but tell me I'm a fool.
    > >
    >
    > I agree that nobody would bother losing 1 swap slot, so it might
    > not be desirable to have this fix. But IMHO, I don't see any reason
    > to leave this discrepancy between regular files and swap devices -- its
    > just so odd.

    Yes, I'd dislike that discrepancy between regular files and block
    devices, if I could see it. Though I'd probably still be cautious
    about the disk partitions.

    dd if=/dev/zero of=/swap bs=200k # says 204800 bytes (205kB)
    mkswap /swap # says size = 196 KiB
    swapon /swap # dmesg says Adding 192k swap

    which is what I've come to expect from the off-by-one,
    even on regular files.

    Hugh


    \
     
     \ /
      Last update: 2009-08-31 21:29    [W:2.972 / U:0.404 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site