Re: [PATCH v7 04/22] Change direct_access calling convention

From: Jan Kara
Date: Mon Mar 31 2014 - 06:28:58 EST


On Sun 23-03-14 15:08:30, Matthew Wilcox wrote:
> In order to support accesses to larger chunks of memory, pass in a
> 'size' parameter (counted in bytes), and return the amount available at
> that address.
>
> Signed-off-by: Matthew Wilcox <matthew.r.wilcox@xxxxxxxxx>
Two minor nits below. Other than that you can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>

> ---
> Documentation/filesystems/xip.txt | 15 +++++++++------
> arch/powerpc/sysdev/axonram.c | 6 +++---
> drivers/block/brd.c | 8 +++++---
> drivers/s390/block/dcssblk.c | 19 ++++++++++---------
> fs/ext2/xip.c | 30 +++++++++++++-----------------
> include/linux/blkdev.h | 4 ++--
> 6 files changed, 42 insertions(+), 40 deletions(-)
>
...
> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index e73b85c..00da60d 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -361,8 +361,8 @@ out:
> }
>
> #ifdef CONFIG_BLK_DEV_XIP
> -static int brd_direct_access(struct block_device *bdev, sector_t sector,
> - void **kaddr, unsigned long *pfn)
> +static long brd_direct_access(struct block_device *bdev, sector_t sector,
> + void **kaddr, unsigned long *pfn, long size)
> {
> struct brd_device *brd = bdev->bd_disk->private_data;
> struct page *page;
> @@ -379,7 +379,9 @@ static int brd_direct_access(struct block_device *bdev, sector_t sector,
> *kaddr = page_address(page);
> *pfn = page_to_pfn(page);
>
> - return 0;
> + /* Could optimistically check to see if the next page in the
> + * file is mapped to the next page of physical RAM */
> + return PAGE_SIZE;
This should be min_t(long, PAGE_SIZE, size), shouldn't it?

> }
> #endif
>
> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> index ebf41e2..da914b2 100644
> --- a/drivers/s390/block/dcssblk.c
> +++ b/drivers/s390/block/dcssblk.c
> @@ -28,8 +28,8 @@
> static int dcssblk_open(struct block_device *bdev, fmode_t mode);
> static void dcssblk_release(struct gendisk *disk, fmode_t mode);
> static void dcssblk_make_request(struct request_queue *q, struct bio *bio);
> -static int dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
> - void **kaddr, unsigned long *pfn);
> +static long dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
> + void **kaddr, unsigned long *pfn, long size);
>
> static char dcssblk_segments[DCSSBLK_PARM_LEN] = "\0";
>
> @@ -866,25 +866,26 @@ fail:
> bio_io_error(bio);
> }
>
> -static int
> +static long
> dcssblk_direct_access (struct block_device *bdev, sector_t secnum,
> - void **kaddr, unsigned long *pfn)
> + void **kaddr, unsigned long *pfn, long size)
> {
> struct dcssblk_dev_info *dev_info;
> - unsigned long pgoff;
> + unsigned long offset, dev_sz;
>
> dev_info = bdev->bd_disk->private_data;
> if (!dev_info)
> return -ENODEV;
> + dev_sz = dev_info->end - dev_info->start;
> if (secnum % (PAGE_SIZE/512))
> return -EINVAL;
> - pgoff = secnum / (PAGE_SIZE / 512);
> - if ((pgoff+1)*PAGE_SIZE-1 > dev_info->end - dev_info->start)
> + offset = secnum * 512;
> + if (offset > dev_sz)
> return -ERANGE;
> - *kaddr = (void *) (dev_info->start+pgoff*PAGE_SIZE);
> + *kaddr = (void *) (dev_info->start + offset);
> *pfn = virt_to_phys(*kaddr) >> PAGE_SHIFT;
>
> - return 0;
> + return min_t(unsigned long, size, dev_sz - offset);
^^^ Why unsigned? Everything seems to be long...

Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/