diff mbox series

[3/9] xfs_copy: actually do directio writes to block devices

Message ID 170069442535.1865809.15981356020247666131.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded, archived
Headers show
Series xfsprogs: minor fixes for 6.6 | expand

Commit Message

Darrick J. Wong Nov. 22, 2023, 11:07 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Not sure why block device targets don't get O_DIRECT in !buffered mode,
but it's misleading when the copy completes instantly only to stall
forever due to fsync-on-close.  Adjust the "write last sector" code to
allocate a properly aligned buffer.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 copy/xfs_copy.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Nov. 23, 2023, 6:40 a.m. UTC | #1 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 79f65946709..26de6b2ee1c 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -854,6 +854,8 @@ main(int argc, char **argv)
>  					progname, target[i].name, progname);
>  				exit(1);
>  			}
> +			if (!buffered_output)
> +				open_flags |= O_DIRECT;
>  		}

I'd just move this outside of the if/else if chain and do the
assignment once.

> @@ -887,20 +889,22 @@ main(int argc, char **argv)
>  				}
>  			}
>  		} else  {
> -			char	*lb[XFS_MAX_SECTORSIZE] = { NULL };
> +			char	*lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
>  			off64_t	off;
>  
>  			/* ensure device files are sufficiently large */
> +			memset(lb, 0, XFS_MAX_SECTORSIZE);
>  
>  			off = mp->m_sb.sb_dblocks * source_blocksize;
> +			off -= XFS_MAX_SECTORSIZE;
> +			if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0)  {

We should probably check for a short write as well?
Also this line is a bit long.
Darrick J. Wong Nov. 27, 2023, 6:14 p.m. UTC | #2 
On Wed, Nov 22, 2023 at 10:40:32PM -0800, Christoph Hellwig wrote:
> > diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> > index 79f65946709..26de6b2ee1c 100644
> > --- a/copy/xfs_copy.c
> > +++ b/copy/xfs_copy.c
> > @@ -854,6 +854,8 @@ main(int argc, char **argv)
> >  					progname, target[i].name, progname);
> >  				exit(1);
> >  			}
> > +			if (!buffered_output)
> > +				open_flags |= O_DIRECT;
> >  		}
> 
> I'd just move this outside of the if/else if chain and do the
> assignment once.

Fixed.

> > @@ -887,20 +889,22 @@ main(int argc, char **argv)
> >  				}
> >  			}
> >  		} else  {
> > -			char	*lb[XFS_MAX_SECTORSIZE] = { NULL };
> > +			char	*lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
> >  			off64_t	off;
> >  
> >  			/* ensure device files are sufficiently large */
> > +			memset(lb, 0, XFS_MAX_SECTORSIZE);
> >  
> >  			off = mp->m_sb.sb_dblocks * source_blocksize;
> > +			off -= XFS_MAX_SECTORSIZE;
> > +			if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0)  {
> 
> We should probably check for a short write as well?
> Also this line is a bit long.

Ok, I'll check for short writes and split the error messaging so that it
no longer says "Is target too small?" on EIO.

--D

> 
> 
>
diff mbox series

Patch

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 79f65946709..26de6b2ee1c 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -854,6 +854,8 @@  main(int argc, char **argv)
 					progname, target[i].name, progname);
 				exit(1);
 			}
+			if (!buffered_output)
+				open_flags |= O_DIRECT;
 		}
 
 		target[i].fd = open(target[i].name, open_flags, 0644);
@@ -887,20 +889,22 @@  main(int argc, char **argv)
 				}
 			}
 		} else  {
-			char	*lb[XFS_MAX_SECTORSIZE] = { NULL };
+			char	*lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
 			off64_t	off;
 
 			/* ensure device files are sufficiently large */
+			memset(lb, 0, XFS_MAX_SECTORSIZE);
 
 			off = mp->m_sb.sb_dblocks * source_blocksize;
-			off -= sizeof(lb);
-			if (pwrite(target[i].fd, lb, sizeof(lb), off) < 0)  {
+			off -= XFS_MAX_SECTORSIZE;
+			if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0)  {
 				do_log(_("%s:  failed to write last block\n"),
 					progname);
 				do_log(_("\tIs target \"%s\" too small?\n"),
 					target[i].name);
 				die_perror();
 			}
+			free(lb);
 		}
 	}