[PATCH v2] kexec/s390x: use mmap instead of read for slurp_file()

[Dave Young]

Hi,

On 10/29/15 at 04:26pm, Michael Holzheu wrote:
> On Thu, 29 Oct 2015 14:37:10 +0800
> Dave Young <dyoung at redhat.com> wrote:
> 
> > On 10/28/15 at 10:57am, Michael Holzheu wrote:
> > > On Wed, 28 Oct 2015 14:46:23 +0800
> > > Dave Young <dyoung at redhat.com> wrote:
> > > 
> > > > Hi, Michael
> > > > 
> > > > > @@ -552,11 +563,18 @@ char *slurp_file(const char *filename, o
> > > > >  		if (err < 0)
> > > > >  			die("Can not seek to the begin of file %s: %s\n",
> > > > >  					filename, strerror(errno));
> > > > > +		buf = slurp_fd(fd, filename, size, &nread, use_mmap);
> > > > >  	} else {
> > > > >  		size = stats.st_size;
> > > > > +		if (use_mmap) {
> > > > > +			buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
> > > > > +				   MAP_PRIVATE, fd, 0);
> > > > > +			nread = stats.st_size;
> > > > > +		} else {
> > > > > +			buf = slurp_fd(fd, filename, size, &nread, 0);
> > > > > +		}
> > > > >  	}
> > > > 
> > > > Drop above changes and replace below lines with an extra use_mmap argument
> > > > should be enough?
> > > > 
> > > > -	buf = slurp_fd(fd, filename, size, &nread);
> > > > [snip]
> > > 
> > > Hmm, I don't think so.
> > > 
> > > In case of non-character devices I either mmap the file directly (use_mmap=true)
> > > or use "slurp_fd()" (use_mmap=false). So I can't unconditionaly use slurp_fd().
> > 
> > How about handle these in slurp_fd only? Directly return mmapped buf in case
> > use_mmap=1 there. I do not understand why use_mmap=1 but you still call read
> > syscall to read data into the mmapped buffer..
> 
> For the character device case (S_ISCHR(stats.st_mode)) we have to use
> the read() syscall path. With my patch I wanted to ensure that when calling
> slurp_file_mmap() we always return mmaped storage.

I understand it now. But it looks odd in code..

> Otherwise slurp_file_mmap() would return mmaped storage for files
> and malloced memory for character devices.
> 
> As already noted this is only to be consistent and is not really required
> for our use case.
> 
> So would you prefer the patch below?

Two nitpicks see the comments inline, otherwise I think it is much better to me.

> 
> Michael
> ---
>  kexec/arch/s390/kexec-image.c |    2 +-
>  kexec/kexec.c                 |   24 +++++++++++++++++++++---
>  kexec/kexec.h                 |    1 +
>  3 files changed, 23 insertions(+), 4 deletions(-)
> 
> --- a/kexec/arch/s390/kexec-image.c
> +++ b/kexec/arch/s390/kexec-image.c
> @@ -101,7 +101,7 @@ image_s390_load(int argc, char **argv, c
>  	 * we load the ramdisk directly behind the image with 1 MiB alignment.
>  	 */
>  	if (ramdisk) {
> -		rd_buffer = slurp_file(ramdisk, &ramdisk_len);
> +		rd_buffer = slurp_file_mmap(ramdisk, &ramdisk_len);
>  		if (rd_buffer == NULL) {
>  			fprintf(stderr, "Could not read ramdisk.\n");
>  			return -1;
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -29,6 +29,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/reboot.h>
> +#include <sys/mman.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #ifndef _O_BINARY
> @@ -514,7 +515,8 @@ static char *slurp_fd(int fd, const char
>  	return buf;
>  }
>  
> -char *slurp_file(const char *filename, off_t *r_size)
> +static char *slurp_file_generic(const char *filename, off_t *r_size,
> +				int use_mmap)

Add a function comment about the argument use_mmap so that one knows
that it will be not used for character devices?

>  {
>  	int fd;
>  	char *buf;
> @@ -552,11 +554,17 @@ char *slurp_file(const char *filename, o
>  		if (err < 0)
>  			die("Can not seek to the begin of file %s: %s\n",
>  					filename, strerror(errno));
> +		buf = slurp_fd(fd, filename, size, &nread);
>  	} else {
>  		size = stats.st_size;
> +		if (use_mmap) {
> +			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +				   MAP_PRIVATE, fd, 0);

Probably map it as readonly is enough.

> +			nread = size;
> +		} else {
> +			buf = slurp_fd(fd, filename, size, &nread);
> +		}
>  	}
> -
> -	buf = slurp_fd(fd, filename, size, &nread);
>  	if (!buf)
>  		die("Cannot read %s", filename);
>  
> @@ -567,6 +575,16 @@ char *slurp_file(const char *filename, o
>  	return buf;
>  }
>  
> +char *slurp_file(const char *filename, off_t *r_size)
> +{
> +	return slurp_file_generic(filename, r_size, 0);
> +}
> +
> +char *slurp_file_mmap(const char *filename, off_t *r_size)
> +{
> +	return slurp_file_generic(filename, r_size, 1);
> +}
> +
>  /* This functions reads either specified number of bytes from the file or
>     lesser if EOF is met. */
>  
> --- a/kexec/kexec.h
> +++ b/kexec/kexec.h
> @@ -253,6 +253,7 @@ extern void die(const char *fmt, ...)
>  extern void *xmalloc(size_t size);
>  extern void *xrealloc(void *ptr, size_t size);
>  extern char *slurp_file(const char *filename, off_t *r_size);
> +extern char *slurp_file_mmap(const char *filename, off_t *r_size);
>  extern char *slurp_file_len(const char *filename, off_t size, off_t *nread);
>  extern char *slurp_decompress_file(const char *filename, off_t *r_size);
>  extern unsigned long virt_to_phys(unsigned long addr);
> 

Thanks
Dave