* [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow
@ 2019-02-24 20:30 Darrick J. Wong
2019-02-24 20:31 ` [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin Darrick J. Wong
2019-02-25 12:55 ` [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Brian Foster
0 siblings, 2 replies; 7+ messages in thread
From: Darrick J. Wong @ 2019-02-24 20:30 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Don't pass raw iomap flags to xfs_reflink_allocate_cow; signal our
intention with a boolean argument.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_iomap.c | 6 +++---
fs/xfs/xfs_reflink.c | 4 ++--
fs/xfs/xfs_reflink.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 01210eae8bf3..a376fb2d4b94 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -983,6 +983,7 @@ xfs_file_iomap_begin(
*/
if (xfs_is_cow_inode(ip)) {
struct xfs_bmbt_irec orig = imap;
+ bool convert_now = (flags & IOMAP_DIRECT);
/* if zeroing doesn't need COW allocation, then we are done. */
if ((flags & IOMAP_ZERO) &&
@@ -991,7 +992,7 @@ xfs_file_iomap_begin(
/* may drop and re-acquire the ilock */
error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
- flags);
+ convert_now);
if (error)
goto out_unlock;
@@ -1002,8 +1003,7 @@ xfs_file_iomap_begin(
* direct I/O code, which must be block aligned we need to
* report the newly allocated address.
*/
- if (!(flags & IOMAP_DIRECT) &&
- orig.br_startblock != HOLESTARTBLOCK)
+ if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
imap = orig;
end_fsb = imap.br_startoff + imap.br_blockcount;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index e2d9179bd50d..d42e3ef9050e 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -361,7 +361,7 @@ xfs_reflink_allocate_cow(
struct xfs_bmbt_irec *imap,
bool *shared,
uint *lockmode,
- unsigned iomap_flags)
+ bool convert_now)
{
struct xfs_mount *mp = ip->i_mount;
xfs_fileoff_t offset_fsb = imap->br_startoff;
@@ -444,7 +444,7 @@ xfs_reflink_allocate_cow(
* to initiate a disk write. For direct I/O we are going to write the
* data and need the conversion, but for buffered writes we're done.
*/
- if (!(iomap_flags & IOMAP_DIRECT) || imap->br_state == XFS_EXT_NORM)
+ if (!convert_now || imap->br_state == XFS_EXT_NORM)
return 0;
trace_xfs_reflink_convert_cow(ip, imap);
return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 2a3052fbe23e..28a43b7f581d 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -27,7 +27,7 @@ bool xfs_inode_need_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode,
- unsigned iomap_flags);
+ bool convert_now);
extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
xfs_off_t count);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin
2019-02-24 20:30 [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Darrick J. Wong
@ 2019-02-24 20:31 ` Darrick J. Wong
2019-02-25 12:55 ` Brian Foster
2019-02-25 12:55 ` [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Brian Foster
1 sibling, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2019-02-24 20:31 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs, bfoster
From: Darrick J. Wong <darrick.wong@oracle.com>
Rework the data flow in xfs_file_iomap_begin where we decide if we have
to break shared extents.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_iomap.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index a376fb2d4b94..a3043b1bcd9a 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -982,7 +982,7 @@ xfs_file_iomap_begin(
* been done up front, so we don't need to do them here.
*/
if (xfs_is_cow_inode(ip)) {
- struct xfs_bmbt_irec orig = imap;
+ struct xfs_bmbt_irec cmap;
bool convert_now = (flags & IOMAP_DIRECT);
/* if zeroing doesn't need COW allocation, then we are done. */
@@ -991,7 +991,8 @@ xfs_file_iomap_begin(
goto out_found;
/* may drop and re-acquire the ilock */
- error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
+ cmap = imap;
+ error = xfs_reflink_allocate_cow(ip, &cmap, &shared, &lockmode,
convert_now);
if (error)
goto out_unlock;
@@ -1001,10 +1002,12 @@ xfs_file_iomap_begin(
* previous block (if there was any) so that the higher level
* write code can perform read-modify-write operations. For
* direct I/O code, which must be block aligned we need to
- * report the newly allocated address.
+ * report the newly allocated address. If the data fork has
+ * a hole, copy the COW fork mapping to avoid allocating to
+ * the data fork.
*/
- if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
- imap = orig;
+ if (convert_now || imap.br_startblock == HOLESTARTBLOCK)
+ imap = cmap;
end_fsb = imap.br_startoff + imap.br_blockcount;
length = XFS_FSB_TO_B(mp, end_fsb) - offset;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow
2019-02-24 20:30 [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Darrick J. Wong
2019-02-24 20:31 ` [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin Darrick J. Wong
@ 2019-02-25 12:55 ` Brian Foster
2019-02-25 16:59 ` Darrick J. Wong
1 sibling, 1 reply; 7+ messages in thread
From: Brian Foster @ 2019-02-25 12:55 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Sun, Feb 24, 2019 at 12:30:58PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Don't pass raw iomap flags to xfs_reflink_allocate_cow; signal our
> intention with a boolean argument.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_iomap.c | 6 +++---
> fs/xfs/xfs_reflink.c | 4 ++--
> fs/xfs/xfs_reflink.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 01210eae8bf3..a376fb2d4b94 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -983,6 +983,7 @@ xfs_file_iomap_begin(
> */
> if (xfs_is_cow_inode(ip)) {
> struct xfs_bmbt_irec orig = imap;
> + bool convert_now = (flags & IOMAP_DIRECT);
>
> /* if zeroing doesn't need COW allocation, then we are done. */
> if ((flags & IOMAP_ZERO) &&
> @@ -991,7 +992,7 @@ xfs_file_iomap_begin(
>
> /* may drop and re-acquire the ilock */
> error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
> - flags);
> + convert_now);
> if (error)
> goto out_unlock;
>
> @@ -1002,8 +1003,7 @@ xfs_file_iomap_begin(
> * direct I/O code, which must be block aligned we need to
> * report the newly allocated address.
> */
> - if (!(flags & IOMAP_DIRECT) &&
> - orig.br_startblock != HOLESTARTBLOCK)
> + if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
FWIW, I thought the original dio check made sense here (in the iomap
callback), whereas the flags parameter to the external function seemed
more of a layering hack. Looks fine, anyways:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> imap = orig;
>
> end_fsb = imap.br_startoff + imap.br_blockcount;
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index e2d9179bd50d..d42e3ef9050e 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -361,7 +361,7 @@ xfs_reflink_allocate_cow(
> struct xfs_bmbt_irec *imap,
> bool *shared,
> uint *lockmode,
> - unsigned iomap_flags)
> + bool convert_now)
> {
> struct xfs_mount *mp = ip->i_mount;
> xfs_fileoff_t offset_fsb = imap->br_startoff;
> @@ -444,7 +444,7 @@ xfs_reflink_allocate_cow(
> * to initiate a disk write. For direct I/O we are going to write the
> * data and need the conversion, but for buffered writes we're done.
> */
> - if (!(iomap_flags & IOMAP_DIRECT) || imap->br_state == XFS_EXT_NORM)
> + if (!convert_now || imap->br_state == XFS_EXT_NORM)
> return 0;
> trace_xfs_reflink_convert_cow(ip, imap);
> return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 2a3052fbe23e..28a43b7f581d 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -27,7 +27,7 @@ bool xfs_inode_need_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
>
> extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
> struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode,
> - unsigned iomap_flags);
> + bool convert_now);
> extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> xfs_off_t count);
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin
2019-02-24 20:31 ` [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin Darrick J. Wong
@ 2019-02-25 12:55 ` Brian Foster
0 siblings, 0 replies; 7+ messages in thread
From: Brian Foster @ 2019-02-25 12:55 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Sun, Feb 24, 2019 at 12:31:05PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Rework the data flow in xfs_file_iomap_begin where we decide if we have
> to break shared extents.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_iomap.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index a376fb2d4b94..a3043b1bcd9a 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -982,7 +982,7 @@ xfs_file_iomap_begin(
> * been done up front, so we don't need to do them here.
> */
> if (xfs_is_cow_inode(ip)) {
> - struct xfs_bmbt_irec orig = imap;
> + struct xfs_bmbt_irec cmap;
> bool convert_now = (flags & IOMAP_DIRECT);
>
> /* if zeroing doesn't need COW allocation, then we are done. */
> @@ -991,7 +991,8 @@ xfs_file_iomap_begin(
> goto out_found;
>
> /* may drop and re-acquire the ilock */
> - error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
> + cmap = imap;
> + error = xfs_reflink_allocate_cow(ip, &cmap, &shared, &lockmode,
> convert_now);
> if (error)
> goto out_unlock;
> @@ -1001,10 +1002,12 @@ xfs_file_iomap_begin(
> * previous block (if there was any) so that the higher level
> * write code can perform read-modify-write operations. For
> * direct I/O code, which must be block aligned we need to
> - * report the newly allocated address.
> + * report the newly allocated address. If the data fork has
> + * a hole, copy the COW fork mapping to avoid allocating to
> + * the data fork.
> */
> - if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
> - imap = orig;
> + if (convert_now || imap.br_startblock == HOLESTARTBLOCK)
> + imap = cmap;
>
> end_fsb = imap.br_startoff + imap.br_blockcount;
> length = XFS_FSB_TO_B(mp, end_fsb) - offset;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow
2019-02-25 12:55 ` [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Brian Foster
@ 2019-02-25 16:59 ` Darrick J. Wong
2019-02-25 17:18 ` Brian Foster
0 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2019-02-25 16:59 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 25, 2019 at 07:55:02AM -0500, Brian Foster wrote:
> On Sun, Feb 24, 2019 at 12:30:58PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Don't pass raw iomap flags to xfs_reflink_allocate_cow; signal our
> > intention with a boolean argument.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/xfs_iomap.c | 6 +++---
> > fs/xfs/xfs_reflink.c | 4 ++--
> > fs/xfs/xfs_reflink.h | 2 +-
> > 3 files changed, 6 insertions(+), 6 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index 01210eae8bf3..a376fb2d4b94 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -983,6 +983,7 @@ xfs_file_iomap_begin(
> > */
> > if (xfs_is_cow_inode(ip)) {
> > struct xfs_bmbt_irec orig = imap;
> > + bool convert_now = (flags & IOMAP_DIRECT);
> >
> > /* if zeroing doesn't need COW allocation, then we are done. */
> > if ((flags & IOMAP_ZERO) &&
> > @@ -991,7 +992,7 @@ xfs_file_iomap_begin(
> >
> > /* may drop and re-acquire the ilock */
> > error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
> > - flags);
> > + convert_now);
> > if (error)
> > goto out_unlock;
> >
> > @@ -1002,8 +1003,7 @@ xfs_file_iomap_begin(
> > * direct I/O code, which must be block aligned we need to
> > * report the newly allocated address.
> > */
> > - if (!(flags & IOMAP_DIRECT) &&
> > - orig.br_startblock != HOLESTARTBLOCK)
> > + if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
>
> FWIW, I thought the original dio check made sense here (in the iomap
> callback), whereas the flags parameter to the external function seemed
> more of a layering hack. Looks fine, anyways:
Yeah, I went back and forth about this 'convert_now'. The flag is
trying to convey that we need to return the disk location of where we
want the write to land.
bool need_real_write_location = (flags & IOMAP_DIRECT);
But that's a long name. "need_write_map", perhaps?
--D
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> > imap = orig;
> >
> > end_fsb = imap.br_startoff + imap.br_blockcount;
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index e2d9179bd50d..d42e3ef9050e 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -361,7 +361,7 @@ xfs_reflink_allocate_cow(
> > struct xfs_bmbt_irec *imap,
> > bool *shared,
> > uint *lockmode,
> > - unsigned iomap_flags)
> > + bool convert_now)
> > {
> > struct xfs_mount *mp = ip->i_mount;
> > xfs_fileoff_t offset_fsb = imap->br_startoff;
> > @@ -444,7 +444,7 @@ xfs_reflink_allocate_cow(
> > * to initiate a disk write. For direct I/O we are going to write the
> > * data and need the conversion, but for buffered writes we're done.
> > */
> > - if (!(iomap_flags & IOMAP_DIRECT) || imap->br_state == XFS_EXT_NORM)
> > + if (!convert_now || imap->br_state == XFS_EXT_NORM)
> > return 0;
> > trace_xfs_reflink_convert_cow(ip, imap);
> > return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> > diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> > index 2a3052fbe23e..28a43b7f581d 100644
> > --- a/fs/xfs/xfs_reflink.h
> > +++ b/fs/xfs/xfs_reflink.h
> > @@ -27,7 +27,7 @@ bool xfs_inode_need_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
> >
> > extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
> > struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode,
> > - unsigned iomap_flags);
> > + bool convert_now);
> > extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> > xfs_off_t count);
> >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow
2019-02-25 16:59 ` Darrick J. Wong
@ 2019-02-25 17:18 ` Brian Foster
2019-02-25 17:43 ` Darrick J. Wong
0 siblings, 1 reply; 7+ messages in thread
From: Brian Foster @ 2019-02-25 17:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Feb 25, 2019 at 08:59:36AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 25, 2019 at 07:55:02AM -0500, Brian Foster wrote:
> > On Sun, Feb 24, 2019 at 12:30:58PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Don't pass raw iomap flags to xfs_reflink_allocate_cow; signal our
> > > intention with a boolean argument.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > fs/xfs/xfs_iomap.c | 6 +++---
> > > fs/xfs/xfs_reflink.c | 4 ++--
> > > fs/xfs/xfs_reflink.h | 2 +-
> > > 3 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > >
> > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > > index 01210eae8bf3..a376fb2d4b94 100644
> > > --- a/fs/xfs/xfs_iomap.c
> > > +++ b/fs/xfs/xfs_iomap.c
> > > @@ -983,6 +983,7 @@ xfs_file_iomap_begin(
> > > */
> > > if (xfs_is_cow_inode(ip)) {
> > > struct xfs_bmbt_irec orig = imap;
> > > + bool convert_now = (flags & IOMAP_DIRECT);
> > >
> > > /* if zeroing doesn't need COW allocation, then we are done. */
> > > if ((flags & IOMAP_ZERO) &&
> > > @@ -991,7 +992,7 @@ xfs_file_iomap_begin(
> > >
> > > /* may drop and re-acquire the ilock */
> > > error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
> > > - flags);
> > > + convert_now);
> > > if (error)
> > > goto out_unlock;
> > >
> > > @@ -1002,8 +1003,7 @@ xfs_file_iomap_begin(
> > > * direct I/O code, which must be block aligned we need to
> > > * report the newly allocated address.
> > > */
> > > - if (!(flags & IOMAP_DIRECT) &&
> > > - orig.br_startblock != HOLESTARTBLOCK)
> > > + if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
> >
> > FWIW, I thought the original dio check made sense here (in the iomap
> > callback), whereas the flags parameter to the external function seemed
> > more of a layering hack. Looks fine, anyways:
>
> Yeah, I went back and forth about this 'convert_now'. The flag is
> trying to convey that we need to return the disk location of where we
> want the write to land.
>
> bool need_real_write_location = (flags & IOMAP_DIRECT);
>
> But that's a long name. "need_write_map", perhaps?
>
What I mean to say is that (IMO) the existing IOMAP_DIRECT checks seemed
fine as long as we changed the param name. FWIW, if we wanted to factor
it out to a bool I'd personally just call it 'direct_io' or 'dio' or
something in xfs_file_iomap_begin() (and leave it as 'convert' or
whatever in the reflink func), but I'm fine with it as is too.
Brian
> --D
>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> >
> > > imap = orig;
> > >
> > > end_fsb = imap.br_startoff + imap.br_blockcount;
> > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > index e2d9179bd50d..d42e3ef9050e 100644
> > > --- a/fs/xfs/xfs_reflink.c
> > > +++ b/fs/xfs/xfs_reflink.c
> > > @@ -361,7 +361,7 @@ xfs_reflink_allocate_cow(
> > > struct xfs_bmbt_irec *imap,
> > > bool *shared,
> > > uint *lockmode,
> > > - unsigned iomap_flags)
> > > + bool convert_now)
> > > {
> > > struct xfs_mount *mp = ip->i_mount;
> > > xfs_fileoff_t offset_fsb = imap->br_startoff;
> > > @@ -444,7 +444,7 @@ xfs_reflink_allocate_cow(
> > > * to initiate a disk write. For direct I/O we are going to write the
> > > * data and need the conversion, but for buffered writes we're done.
> > > */
> > > - if (!(iomap_flags & IOMAP_DIRECT) || imap->br_state == XFS_EXT_NORM)
> > > + if (!convert_now || imap->br_state == XFS_EXT_NORM)
> > > return 0;
> > > trace_xfs_reflink_convert_cow(ip, imap);
> > > return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> > > diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> > > index 2a3052fbe23e..28a43b7f581d 100644
> > > --- a/fs/xfs/xfs_reflink.h
> > > +++ b/fs/xfs/xfs_reflink.h
> > > @@ -27,7 +27,7 @@ bool xfs_inode_need_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
> > >
> > > extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
> > > struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode,
> > > - unsigned iomap_flags);
> > > + bool convert_now);
> > > extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> > > xfs_off_t count);
> > >
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow
2019-02-25 17:18 ` Brian Foster
@ 2019-02-25 17:43 ` Darrick J. Wong
0 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2019-02-25 17:43 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 25, 2019 at 12:18:54PM -0500, Brian Foster wrote:
> On Mon, Feb 25, 2019 at 08:59:36AM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 25, 2019 at 07:55:02AM -0500, Brian Foster wrote:
> > > On Sun, Feb 24, 2019 at 12:30:58PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > >
> > > > Don't pass raw iomap flags to xfs_reflink_allocate_cow; signal our
> > > > intention with a boolean argument.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > > fs/xfs/xfs_iomap.c | 6 +++---
> > > > fs/xfs/xfs_reflink.c | 4 ++--
> > > > fs/xfs/xfs_reflink.h | 2 +-
> > > > 3 files changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > >
> > > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > > > index 01210eae8bf3..a376fb2d4b94 100644
> > > > --- a/fs/xfs/xfs_iomap.c
> > > > +++ b/fs/xfs/xfs_iomap.c
> > > > @@ -983,6 +983,7 @@ xfs_file_iomap_begin(
> > > > */
> > > > if (xfs_is_cow_inode(ip)) {
> > > > struct xfs_bmbt_irec orig = imap;
> > > > + bool convert_now = (flags & IOMAP_DIRECT);
> > > >
> > > > /* if zeroing doesn't need COW allocation, then we are done. */
> > > > if ((flags & IOMAP_ZERO) &&
> > > > @@ -991,7 +992,7 @@ xfs_file_iomap_begin(
> > > >
> > > > /* may drop and re-acquire the ilock */
> > > > error = xfs_reflink_allocate_cow(ip, &imap, &shared, &lockmode,
> > > > - flags);
> > > > + convert_now);
> > > > if (error)
> > > > goto out_unlock;
> > > >
> > > > @@ -1002,8 +1003,7 @@ xfs_file_iomap_begin(
> > > > * direct I/O code, which must be block aligned we need to
> > > > * report the newly allocated address.
> > > > */
> > > > - if (!(flags & IOMAP_DIRECT) &&
> > > > - orig.br_startblock != HOLESTARTBLOCK)
> > > > + if (!convert_now && orig.br_startblock != HOLESTARTBLOCK)
> > >
> > > FWIW, I thought the original dio check made sense here (in the iomap
> > > callback), whereas the flags parameter to the external function seemed
> > > more of a layering hack. Looks fine, anyways:
> >
> > Yeah, I went back and forth about this 'convert_now'. The flag is
> > trying to convey that we need to return the disk location of where we
> > want the write to land.
> >
> > bool need_real_write_location = (flags & IOMAP_DIRECT);
> >
> > But that's a long name. "need_write_map", perhaps?
> >
>
> What I mean to say is that (IMO) the existing IOMAP_DIRECT checks seemed
> fine as long as we changed the param name. FWIW, if we wanted to factor
> it out to a bool I'd personally just call it 'direct_io' or 'dio' or
> something in xfs_file_iomap_begin() (and leave it as 'convert' or
> whatever in the reflink func), but I'm fine with it as is too.
<nod> I'll change 'convert_now' -> 'directio' in this function. The
reflink function will retain the 'convert_now' name.
--D
> Brian
>
> > --D
> >
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > >
> > > > imap = orig;
> > > >
> > > > end_fsb = imap.br_startoff + imap.br_blockcount;
> > > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > > index e2d9179bd50d..d42e3ef9050e 100644
> > > > --- a/fs/xfs/xfs_reflink.c
> > > > +++ b/fs/xfs/xfs_reflink.c
> > > > @@ -361,7 +361,7 @@ xfs_reflink_allocate_cow(
> > > > struct xfs_bmbt_irec *imap,
> > > > bool *shared,
> > > > uint *lockmode,
> > > > - unsigned iomap_flags)
> > > > + bool convert_now)
> > > > {
> > > > struct xfs_mount *mp = ip->i_mount;
> > > > xfs_fileoff_t offset_fsb = imap->br_startoff;
> > > > @@ -444,7 +444,7 @@ xfs_reflink_allocate_cow(
> > > > * to initiate a disk write. For direct I/O we are going to write the
> > > > * data and need the conversion, but for buffered writes we're done.
> > > > */
> > > > - if (!(iomap_flags & IOMAP_DIRECT) || imap->br_state == XFS_EXT_NORM)
> > > > + if (!convert_now || imap->br_state == XFS_EXT_NORM)
> > > > return 0;
> > > > trace_xfs_reflink_convert_cow(ip, imap);
> > > > return xfs_reflink_convert_cow_locked(ip, offset_fsb, count_fsb);
> > > > diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> > > > index 2a3052fbe23e..28a43b7f581d 100644
> > > > --- a/fs/xfs/xfs_reflink.h
> > > > +++ b/fs/xfs/xfs_reflink.h
> > > > @@ -27,7 +27,7 @@ bool xfs_inode_need_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap,
> > > >
> > > > extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
> > > > struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode,
> > > > - unsigned iomap_flags);
> > > > + bool convert_now);
> > > > extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> > > > xfs_off_t count);
> > > >
> > > >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-25 17:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-24 20:30 [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Darrick J. Wong
2019-02-24 20:31 ` [PATCH 2/2] xfs: rework breaking of shared extents in xfs_file_iomap_begin Darrick J. Wong
2019-02-25 12:55 ` Brian Foster
2019-02-25 12:55 ` [PATCH 1/2] xfs: don't pass iomap flags to xfs_reflink_allocate_cow Brian Foster
2019-02-25 16:59 ` Darrick J. Wong
2019-02-25 17:18 ` Brian Foster
2019-02-25 17:43 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.