Re: [PATCH] [45/48] Suspend2 2.1.9.8 for 2.6.12: 621-swsusp-tidy.patch

[Pekka Enberg <penberg@gmail.com>]

On 7/6/05, Nigel Cunningham <nigel@suspend2.net> wrote:
> diff -ruNp 622-swapwriter.patch-old/kernel/power/suspend_swap.c 622-swapwriter.patch-new/kernel/power/suspend_swap.c
> --- 622-swapwriter.patch-old/kernel/power/suspend_swap.c        1970-01-01 10:00:00.000000000 +1000
> +++ 622-swapwriter.patch-new/kernel/power/suspend_swap.c        2005-07-05 23:48:59.000000000 +1000
> +/*
> + * ---------------------------------------------------------------
> + *
> + *     Internal Data Structures
> + *
> + * ---------------------------------------------------------------
> + */

Can we please tone down this banner?

> +struct {
> +       /* Extent chains for swap & blocks */
> +       struct extentchain swapextents;
> +       struct extentchain block_chain[MAX_SWAPFILES];
> +
> +       /* Location of start of pagedir 1 */
> +       struct extent * pd1start_block_extent;
> +       int pd1start_chain;
> +       int pd1start_extent_number;
> +       unsigned long pd1start_block_offset;
> +
> +       /* Devices used for swap */
> +       dev_t swapdevs[MAX_SWAPFILES];
> +       char blocksizes[MAX_SWAPFILES];
> +
> +} header_data;
> +
> +static dev_t header_device = 0;
> +static struct block_device * header_block_device = NULL;
> +static int headerblocksize = PAGE_SIZE;
> +static int headerblock;
> +
> +/* For swapfile automatically swapon/off'd. */
> +static char swapfilename[256] = "";

<linux/suspend.h> already has SWAP_FILENAME_MAXLENGTH. Use that.

> +extern asmlinkage long sys_swapon(const char * specialfile, int swap_flags);
> +extern asmlinkage long sys_swapoff(const char * specialfile);
> +static int suspend_swapon_status = 0;
> +
> +/*
> + * ---------------------------------------------------------------
> + *
> + *     Current state.
> + *
> + * ---------------------------------------------------------------
> + */

Could we please tone down this banner?

> +
> +/* Which pagedir are we saving/reloading? Needed so we can know whether to
> + * remember the last swap entry used at the end of writing pageset2, and
> + * get that location when saving or reloading pageset1.*/
> +static int current_stream = 0;

No need to set to zero. The compiler will do it for you.

> +
> +/* Pointer to current swap entry being loaded/saved. */
> +static struct extent * currentblockextent = NULL;
> +static unsigned long currentblockoffset = 0;
> +static int currentblockchain = 0;
> +static int currentblocksperpage = 0;
> +
> +/* Header Page Information */
> +static int header_pages_allocated = 0;
> +static struct submit_params * first_header_submit_info = NULL,
> + * last_header_submit_info = NULL, * current_header_submit_info = NULL;

Ditto. (applies to NULL as well)

> +static void get_header_params(struct submit_params * headerpage)
> +{
> +       swp_entry_t entry = headerpage->swap_address;
> +       int swapfilenum = swp_type(entry);
> +       unsigned long offset = swp_offset(entry);
> +       struct swap_info_struct * sis = get_swap_info_struct(swapfilenum);
> +       sector_t sector = map_swap_page(sis, offset);
> +
> +       headerpage->dev = sis->bdev,
> +       headerpage->block[0] = sector;
> +       //headerpage->blocks_used = 1;

Please drop commented out code.

> +static int try_to_open_resume_device(void)
> +{
> +       resume_block_device = open_by_devnum(resume_device, FMODE_READ);
> +
> +       if (IS_ERR(resume_block_device) || (!resume_block_device))

The second set of parenthesis are redundant.

> +       /*
> +        * Put bdev of suspend header in last byte of swap header
> +        * (unsigned short)
> +        */
> +       if (type > 11) {
> +               dev_t * header_ptr = (dev_t *) &header[1];
> +               unsigned char * headerblocksize_ptr =
> +                       (unsigned char *) &header[5];
> +               unsigned long * headerblock_ptr = (unsigned long *) &header[6];
> +               header_device = *header_ptr;
> +               /*
> +                * We are now using the highest bit of the char to indicate
> +                * whether we have attempted to resume from this image before.
> +                */
> +               clear_suspend_state(SUSPEND_RESUMED_BEFORE);
> +               if (((int) *headerblocksize_ptr) & 0x80)
> +                       set_suspend_state(SUSPEND_RESUMED_BEFORE);
> +               headerblocksize = 512 * (((int) *headerblocksize_ptr) & 0xf);

Hardcoded sector size?

> +
> +       /* Restore device info */
> +       for (i = 0; i < MAX_SWAPFILES; i++) {
> +               dev_t thisdevice = header_data.swapdevs[i];
> +
> +               swap_info[i].bdev = NULL;
> +
> +               if (!thisdevice)
> +                       continue;
> +
> +               if (thisdevice == resume_device) {
> +                       swap_info[i].bdev = resume_block_device;
> +                       /* Mark as used so the device doesn't get suspended. */
> +                       swap_info[i].swap_file = (struct file *) 0xffffff;

Please use constants instead of magic numbers.

> +static int swapwriter_write_init(int stream_number)
> +{
> +       current_stream = stream_number;
> +
> +       if (current_stream == 1) {
> +               currentblockextent = header_data.pd1start_block_extent;
> +               currentblockoffset = header_data.pd1start_block_offset;
> +               currentblockchain = header_data.pd1start_chain;
> +       } else

See below.

> +               for (currentblockchain = 0; currentblockchain < MAX_SWAPFILES;
> +                               currentblockchain++)
> +                       if (header_data.block_chain[currentblockchain].first) {
> +                               currentblockextent =
> +                                       header_data.
> +                                        block_chain[currentblockchain].first;
> +                               currentblockoffset = currentblockextent->minimum;
> +                               break;
> +                       }

Braces would make sense here.


> +
> +       currentblocksperpage = get_blocks_per_page(currentblockchain);
> +
> +       suspend_bio_ops.reset_io_stats();
> +
> +       return 0;
> +}
> +
> +static int swapwriter_write_chunk(struct page * buffer_page)
> +{
> +       int i;
> +       struct submit_params submit_params;
> +
> +       if (TEST_ACTION_STATE(SUSPEND_TEST_FILTER_SPEED))
> +               return 0;
> +
> +       if (currentblockchain == MAX_SWAPFILES) {
> +               printk("Error! We have run out of blocks for writing data.\n");
> +               return -ENOSPC;
> +       }
> +
> +       if (!currentblockextent) {
> +               do {
> +                       currentblockchain++;
> +               } while ((currentblockchain < MAX_SWAPFILES) &&
> +                  (!header_data.block_chain[currentblockchain].first));
> +
> +               /* We can validly not have a new blockextent. We
> +                * might be compressing data and the user was
> +                * too optimistic in setting the compression
> +                * ratio or we're just copying the pageset. */
> +
> +               if (currentblockchain == MAX_SWAPFILES) {
> +                       printk("Argh. Ran out of block chains.\n");
> +                       return -ENOSPC;
> +               }
> +
> +               currentblockextent =
> +                header_data.block_chain[currentblockchain].first;
> +               currentblockoffset = currentblockextent->minimum;
> +               currentblocksperpage = get_blocks_per_page(currentblockchain);
> +       }
> +
> +       submit_params.readahead_index = -1;
> +       submit_params.page = buffer_page;
> +       submit_params.dev = swap_info[currentblockchain].bdev;
> +
> +       /* Get the blocks */
> +       submit_params.block[0] = currentblockoffset;
> +       for (i = 0; i < currentblocksperpage; i++)
> +               GET_EXTENT_NEXT(currentblockextent, currentblockoffset);
> +
> +       suspend_bio_ops.submit_io(WRITE, &submit_params, 0);
> +
> +       check_shift_keys(0, NULL);
> +
> +       return 0;
> +}
> +
> +static int swapwriter_write_cleanup(void)
> +{
> +       if (current_stream == 2) {

Where did that magic number two come from?

> +               header_data.pd1start_block_extent = currentblockextent;
> +               header_data.pd1start_block_offset = currentblockoffset;
> +               header_data.pd1start_chain = currentblockchain;
> +       }
> +
> +       suspend_bio_ops.finish_all_io();
> +
> +       suspend_bio_ops.check_io_stats();
> +
> +       return 0;
> +}
> +
> +static int swapwriter_read_init(int stream_number)
> +{
> +       current_stream = stream_number;
> +
> +       if (current_stream == 1) {

Ditto.

> +static int swapwriter_parse_image_location(char * commandline, int only_writer)
> +{
> +       char *thischar, *devstart = NULL, *colon = NULL, *at_symbol = NULL;
> +       union p_diskpage diskpage;
> +       int signature_found, result = -EINVAL, temp_result;
> +
> +       if (strncmp(commandline, "swap:", 5)) {
> +               if (!only_writer)
> +                       return 1;
> +       } else
> +               commandline += 5;
> +
> +       devstart = thischar = commandline;
> +       while ((*thischar != ':') && (*thischar != '@') &&
> +               ((thischar - commandline) < 250) && (*thischar))

What's the magic 250 here?

> +               thischar++;
> +
> +       if (*thischar == ':') {
> +               colon = thischar;
> +               *colon = 0;
> +               thischar++;
> +       }
> +
> +       while ((*thischar != '@') && ((thischar - commandline) < 250) && (*thischar))

And here?

> +               thischar++;
> +
> +       if (*thischar == '@') {
> +               at_symbol = thischar;
> +               *at_symbol = 0;
> +       }
> +
> +       if (colon)
> +               resume_firstblock = (int) simple_strtoul(colon + 1, NULL, 0);
> +       else
> +               resume_firstblock = 0;
> +
> +       if (at_symbol) {
> +               resume_firstblocksize = (int) simple_strtoul(at_symbol + 1, NULL, 0);
> +               if (resume_firstblocksize & 0x1FF)
> +                       printk("Swapwriter: Blocksizes are usually a multiple of 512. Don't expect this to work!\n");

Then why do we allow it?