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

[Jan Kara]

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/