Re: [PATCH 2/4] kpartx: read devices with direct IO

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2020-07-01 at 17:39 -0500, Benjamin Marzinski wrote:
> If kpartx is used on top of shared storage, and a device has its
> partition table changed on one machine, and then kpartx is run on
> another, it may not see the new data, because the cache still
> contains
> the old data, and there is nothing to tell the machine running kpartx
> to
> invalidate it. To solve this, kpartx should read the devices using
> direct io.
> 
> One issue with how this code has been updated is that the original
> code
> for getblock() always read 1024 bytes. The new code reads a logical
> sector size chunk of the device, and returns a pointer to the 512
> byte
> sector that the caller asked for, within that (possibly larger)
> chunk.
> This means that if the logical sector size is 512, then the code is
> now
> only reading 512 bytes.  Looking through the code for the various
> partition types, I can't see a case where more than 512 bytes is
> needed
> and getblock() is used.  If anyone has a reason why this code should
> be
> reading 1024 bytes at minmum, I can certainly change this.  But when
> I
> looked, I couldn't find a case where reading 512 bytes would cause a
> problem.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>

The value 1024 comes from first partx in util-linux, where Christophe 
pulled the first kpartx from: 
http://ftp.be.debian.org/pub/linux/utils/util-linux/v2.10/util-linux-2.10m.tar.gz 
I, too, see no reason for reading more than a single block.

Two nits below. Here's one more, I'd have preferred two separate
patches for these two different issues. But never mind.

Regards,
Martin

> ---
>  kpartx/dasd.c   |  7 ++++---
>  kpartx/gpt.c    | 22 ++++++++++------------
>  kpartx/kpartx.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  kpartx/kpartx.h |  2 ++
>  4 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> index 14b9d3aa..f0398645 100644
> --- a/kpartx/dasd.c
> +++ b/kpartx/dasd.c
> @@ -22,6 +22,7 @@
>   * along with this program.  If not, see <
> http://www.gnu.org/licenses/>;.
>   */
>  
> +#define _GNU_SOURCE
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -117,13 +118,13 @@ read_dasd_pt(int fd, __attribute__((unused))
> struct slice all,
>  
>  		sprintf(pathname, "/dev/.kpartx-node-%u-%u",
>  			(unsigned int)major(dev), (unsigned
> int)minor(dev));
> -		if ((fd_dasd = open(pathname, O_RDONLY)) == -1) {
> +		if ((fd_dasd = open(pathname, O_RDONLY | O_DIRECT)) ==
> -1) {
>  			/* Devicenode does not exist. Try to create one
> */
>  			if (mknod(pathname, 0600 | S_IFBLK, dev) == -1)
> {
>  				/* Couldn't create a device node */
>  				return -1;
>  			}
> -			fd_dasd = open(pathname, O_RDONLY);
> +			fd_dasd = open(pathname, O_RDONLY | O_DIRECT);
>  			/*
>  			 * The file will vanish when the last process
> (we)
>  			 * has ceased to access it.
> @@ -175,7 +176,7 @@ read_dasd_pt(int fd, __attribute__((unused))
> struct slice all,
>  	 * Get volume label, extract name and type.
>  	 */
>  
> -	if (!(data = (unsigned char *)malloc(blocksize)))
> +	if (aligned_malloc((void **)&data, blocksize, NULL))
>  		goto out;
>  
>  
> diff --git a/kpartx/gpt.c b/kpartx/gpt.c
> index 785b34ea..f7fefb70 100644
> --- a/kpartx/gpt.c
> +++ b/kpartx/gpt.c
> @@ -243,8 +243,7 @@ alloc_read_gpt_entries(int fd, gpt_header * gpt)
>  
>  	if (!count) return NULL;
>  
> -	pte = (gpt_entry *)malloc(count);
> -	if (!pte)
> +	if (aligned_malloc((void **)&pte, get_sector_size(fd), &count))
>  		return NULL;
>  	memset(pte, 0, count);
>  
> @@ -269,12 +268,11 @@ static gpt_header *
>  alloc_read_gpt_header(int fd, uint64_t lba)
>  {
>  	gpt_header *gpt;
> -	gpt = (gpt_header *)
> -	    malloc(sizeof (gpt_header));
> -	if (!gpt)
> +	size_t size = sizeof (gpt_header);
> +	if (aligned_malloc((void **)&gpt, get_sector_size(fd), &size))
>  		return NULL;
> -	memset(gpt, 0, sizeof (*gpt));
> -	if (!read_lba(fd, lba, gpt, sizeof (gpt_header))) {
> +	memset(gpt, 0, size);
> +	if (!read_lba(fd, lba, gpt, size)) {
>  		free(gpt);
>  		return NULL;
>  	}
> @@ -498,6 +496,7 @@ find_valid_gpt(int fd, gpt_header ** gpt,
> gpt_entry ** ptes)
>  	gpt_header *pgpt = NULL, *agpt = NULL;
>  	gpt_entry *pptes = NULL, *aptes = NULL;
>  	legacy_mbr *legacymbr = NULL;
> +	size_t size = sizeof(legacy_mbr);
>  	uint64_t lastlba;
>  	if (!gpt || !ptes)
>  		return 0;
> @@ -526,11 +525,10 @@ find_valid_gpt(int fd, gpt_header ** gpt,
> gpt_entry ** ptes)
>  	}
>  
>  	/* This will be added to the EFI Spec. per Intel after v1.02.
> */
> -	legacymbr = malloc(sizeof (*legacymbr));
> -	if (legacymbr) {
> -		memset(legacymbr, 0, sizeof (*legacymbr));
> -		read_lba(fd, 0, (uint8_t *) legacymbr,
> -			 sizeof (*legacymbr));
> +	if (aligned_malloc((void **)&legacymbr, get_sector_size(fd),
> +			   &size) == 0) {
> +		memset(legacymbr, 0, size);
> +		read_lba(fd, 0, (uint8_t *) legacymbr, size);
>  		good_pmbr = is_pmbr_valid(legacymbr);
>  		free(legacymbr);
>  		legacymbr=NULL;
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index d3620c5c..4716dd4d 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -19,6 +19,7 @@
>   * cva, 2002-10-26
>   */
>  
> +#define _GNU_SOURCE
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <errno.h>
> @@ -41,7 +42,6 @@
>  
>  #define SIZE(a) (sizeof(a)/sizeof((a)[0]))
>  
> -#define READ_SIZE	1024
>  #define MAXTYPES	64
>  #define MAXSLICES	256
>  #define DM_TARGET	"linear"
> @@ -388,7 +388,7 @@ main(int argc, char **argv){
>  		set_delimiter(mapname, delim);
>  	}
>  
> -	fd = open(device, O_RDONLY);
> +	fd = open(device, O_RDONLY | O_DIRECT);
>  
>  	if (fd == -1) {
>  		perror(device);
> @@ -690,9 +690,9 @@ xmalloc (size_t size) {
>   */
>  
>  static int
> -sseek(int fd, unsigned int secnr) {
> +sseek(int fd, unsigned int secnr, int secsz) {
>  	off64_t in, out;
> -	in = ((off64_t) secnr << 9);
> +	in = ((off64_t) secnr * secsz);
>  	out = 1;
>  
>  	if ((out = lseek64(fd, in, SEEK_SET)) != in)
> @@ -703,6 +703,21 @@ sseek(int fd, unsigned int secnr) {
>  	return 0;
>  }
>  
> +int
> +aligned_malloc(void **mem_p, size_t align, size_t *size_p)
> +{
> +	size_t pgsize = getpagesize();

Nit: I'd use a static variable here and call getpagesize() only once.

> +	size_t size = pgsize;
> +	if (!mem_p || !align || (size_p && !*size_p))
> +		return EINVAL;
> +
> +	if (size_p)
> +		*size_p = size = ((*size_p + align - 1) / align) *
> align;

It would be cleaner to set *size_p only after successful return from
posix_memalign().

> +
> +	return posix_memalign(mem_p, pgsize, size);
> +}




> +
> +/* always in sector size blocks */
>  static
>  struct block {
>  	unsigned int secnr;
> @@ -710,30 +725,39 @@ struct block {
>  	struct block *next;
>  } *blockhead;
>  
> +/* blknr is always in 512 byte blocks */
>  char *
> -getblock (int fd, unsigned int secnr) {
> +getblock (int fd, unsigned int blknr) {
> +	unsigned int secsz = get_sector_size(fd);
> +	unsigned int blks_per_sec = secsz / 512;
> +	unsigned int secnr = blknr / blks_per_sec;
> +	unsigned int blk_off = (blknr % blks_per_sec) * 512;
>  	struct block *bp;
>  
>  	for (bp = blockhead; bp; bp = bp->next)
>  
>  		if (bp->secnr == secnr)
> -			return bp->block;
> +			return bp->block + blk_off;
>  
> -	if (sseek(fd, secnr))
> +	if (sseek(fd, secnr, secsz))
>  		return NULL;
>  
>  	bp = xmalloc(sizeof(struct block));
>  	bp->secnr = secnr;
>  	bp->next = blockhead;
>  	blockhead = bp;
> -	bp->block = (char *) xmalloc(READ_SIZE);
> +	if (aligned_malloc((void **)&bp->block, secsz, NULL)) {
> +		fprintf(stderr, "aligned_malloc failed\n");
> +		exit(1);
> +	}
>  
> -	if (read(fd, bp->block, READ_SIZE) != READ_SIZE) {
> +	if (read(fd, bp->block, secsz) != secsz) {
>  		fprintf(stderr, "read error, sector %d\n", secnr);
> -		bp->block = NULL;
> +		blockhead = bp->next;
> +		return NULL;
>  	}
>  
> -	return bp->block;
> +	return bp->block + blk_off;
>  }
>  
>  int
> diff --git a/kpartx/kpartx.h b/kpartx/kpartx.h
> index 67edeb82..727632c1 100644
> --- a/kpartx/kpartx.h
> +++ b/kpartx/kpartx.h
> @@ -1,6 +1,7 @@
>  #ifndef _KPARTX_H
>  #define _KPARTX_H
>  
> +#include <stddef.h>
>  #include <stdint.h>
>  #include <sys/ioctl.h>
>  
> @@ -61,6 +62,7 @@ extern ptreader read_mac_pt;
>  extern ptreader read_sun_pt;
>  extern ptreader read_ps3_pt;
>  
> +int aligned_malloc(void **mem_p, size_t align, size_t *size_p);
>  char *getblock(int fd, unsigned int secnr);
>  
>  static inline unsigned int

-- 
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer



--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux