Re: [PATCH] [46/48] Suspend2 2.1.9.8 for 2.6.12: 622-swapwriter.patch

[Pekka Enberg <penberg@gmail.com>]

On 7/6/05, Nigel Cunningham <nigel@suspend2.net> wrote:
> diff -ruNp 623-generic-block-io.patch-old/kernel/power/suspend_block_io.c 623-generic-block-io.patch-new/kernel/power/suspend_block_io.c
> --- 623-generic-block-io.patch-old/kernel/power/suspend_block_io.c      1970-01-01 10:00:00.000000000 +1000
> +++ 623-generic-block-io.patch-new/kernel/power/suspend_block_io.c      2005-07-05 23:48:59.000000000 +1000
> @@ -0,0 +1,817 @@
> +#include "suspend2_core/suspend.h"
> +#include "suspend2_core/proc.h"
> +#include "suspend2_core/plugins.h"
> +#include "suspend2_core/utility.h"
> +#include "suspend2_core/prepare_image.h"

You've got circular dependencies. kernel/power/suspend2_core depends
on kernel/power and vice versa. Please just consider putting them all
in kernel/power/

> +/* Bits in struct io_info->flags */
> +#define IO_WRITING 1
> +#define IO_RESTORE_PAGE_PROT 2
> +#define IO_AWAITING_READ 3
> +#define IO_AWAITING_WRITE 4
> +#define IO_AWAITING_SUBMIT 5
> +#define IO_AWAITING_CLEANUP 6
> +#define IO_HANDLE_PAGE_PROT 7

Please use enums instead.

> +
> +#define MAX_OUTSTANDING_IO 1024
> +
> +/*
> + * ---------------------------------------------------------------
> + *
> + *     IO in progress information storage and helpers
> + *
> + * ---------------------------------------------------------------
> + */

Could we please drop the above banner. Hurts my eyes...

> +
> +struct io_info {
> +       struct bio * sys_struct;
> +       long block[PAGE_SIZE/512];

You're hardcoding sector size here, no?

> +       struct page * buffer_page;
> +       struct page * data_page;
> +       unsigned long flags;
> +       struct block_device * dev;
> +       struct list_head list;
> +       int readahead_index;
> +       struct work_struct work;
> +};
> +
> +/* Locks separated to allow better SMP support.
> + * An io_struct moves through the lists as follows.
> + * free -> submit_batch -> busy -> ready_for_cleanup -> free
> + */
> +static LIST_HEAD(ioinfo_free);
> +static spinlock_t ioinfo_free_lock = SPIN_LOCK_UNLOCKED;

Please use DEFINE_SPINLOCK instead. It is preferred as some automatic
lock checkers need it.

> +#define BITS_PER_UL (8 * sizeof(unsigned long))
> +static volatile unsigned long suspend_readahead_flags[(MAX_READAHEAD + BITS_PER_UL - 1) / BITS_PER_UL];

<asm/types.h> has BITS_PER_LONG. Use it.

> +static int suspend_end_bio(struct bio * bio, unsigned int num, int err)
> +{
> +       struct io_info *io_info = (struct io_info *) bio->bi_private;

Redundant cast.

> +static void suspend_wait_on_readahead(int readahead_index)
> +{
> +       int index = readahead_index/(8 * sizeof(unsigned long));
> +       int bit = readahead_index - index * 8 * sizeof(unsigned long);

Use BITS_PER_LONG here.

> +
> +       /* read_ahead_index is the one we want to return */
> +       while (!test_bit(bit, &suspend_readahead_flags[index]))
> +               do_bio_wait(6);
> +}
> +
> +/*
> + * readahead_done
> + *
> + * Returns whether the readahead requested is ready.
> + */
> +
> +static int suspend_readahead_ready(int readahead_index)
> +{
> +       int index = readahead_index/(8 * sizeof(unsigned long));
> +       int bit = readahead_index - (index * 8 * sizeof(unsigned long));

Ditto.

> +
> +       return test_bit(bit, &suspend_readahead_flags[index]);
> +}
> +
> +/* suspend_readahead_prepare
> + * Set up for doing readahead on an image */
> +static int suspend_prepare_readahead(int index)
> +{
> +       unsigned long new_page = get_zeroed_page(GFP_ATOMIC);
> +
> +       if(!new_page)
> +               return -ENOMEM;
> +
> +       suspend_bio_ops.readahead_pages[index] = virt_to_page(new_page);
> +       return 0;
> +}
> +
> +/* suspend_readahead_cleanup
> + * Clean up structures used for readahead */
> +static void suspend_cleanup_readahead(int page)
> +{
> +       __free_pages(suspend_bio_ops.readahead_pages[page], 0);
> +       suspend_bio_ops.readahead_pages[page] = 0;
> +       return;
> +}
> +
> +static unsigned long suspend_bio_memory_needed(void)
> +{
> +       /* We want to have at least enough memory so as to have 128 I/O
> +        * transactions on the fly at once. If we can to more, fine. */
> +       return (128 * (PAGE_SIZE + sizeof(struct request) +
> +                               sizeof(struct bio) + sizeof(struct io_info)));

Where did the magic 128 come from?