| Message ID | 20191025150336.19411-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] xfs: simplify xfs_iomap_eof_align_last_fsb | expand |
On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote: > Even if we are asked for a write layout there is no point in logging > the inode unless we actually modified it in some way. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------ > 1 file changed, 19 insertions(+), 24 deletions(-) > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c > index 9c96493be9e0..fa90c6334c7c 100644 > --- a/fs/xfs/xfs_pnfs.c > +++ b/fs/xfs/xfs_pnfs.c > @@ -147,32 +147,27 @@ xfs_fs_map_blocks( > if (error) > goto out_unlock; > > - if (write) { > - enum xfs_prealloc_flags flags = 0; > - > + if (write && > + (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) { > ASSERT(imap.br_startblock != DELAYSTARTBLOCK); The change in code flow makes this assert rather useless, I think, since we only end up in this branch if we have a write and a hole. If the condition that it checks is important (and it seems to be?) then it ought to be hoisted up a level and turned into: ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK); Right? Otherwise looks ok. --D > > - if (!nimaps || imap.br_startblock == HOLESTARTBLOCK) { > - /* > - * xfs_iomap_write_direct() expects to take ownership of > - * the shared ilock. > - */ > - xfs_ilock(ip, XFS_ILOCK_SHARED); > - error = xfs_iomap_write_direct(ip, offset, length, > - &imap, nimaps); > - if (error) > - goto out_unlock; > - > - /* > - * Ensure the next transaction is committed > - * synchronously so that the blocks allocated and > - * handed out to the client are guaranteed to be > - * present even after a server crash. > - */ > - flags |= XFS_PREALLOC_SET | XFS_PREALLOC_SYNC; > - } > - > - error = xfs_update_prealloc_flags(ip, flags); > + /* > + * xfs_iomap_write_direct() expects to take ownership of the > + * shared ilock. > + */ > + xfs_ilock(ip, XFS_ILOCK_SHARED); > + error = xfs_iomap_write_direct(ip, offset, length, &imap, > + nimaps); > + if (error) > + goto out_unlock; > + > + /* > + * Ensure the next transaction is committed synchronously so > + * that the blocks allocated and handed out to the client are > + * guaranteed to be present even after a server crash. > + */ > + error = xfs_update_prealloc_flags(ip, > + XFS_PREALLOC_SET | XFS_PREALLOC_SYNC); > if (error) > goto out_unlock; > } > -- > 2.20.1 >
On Mon, Oct 28, 2019 at 09:12:45AM -0700, Darrick J. Wong wrote: > On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote: > > Even if we are asked for a write layout there is no point in logging > > the inode unless we actually modified it in some way. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------ > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c > > index 9c96493be9e0..fa90c6334c7c 100644 > > --- a/fs/xfs/xfs_pnfs.c > > +++ b/fs/xfs/xfs_pnfs.c > > @@ -147,32 +147,27 @@ xfs_fs_map_blocks( > > if (error) > > goto out_unlock; > > > > - if (write) { > > - enum xfs_prealloc_flags flags = 0; > > - > > + if (write && > > + (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) { > > ASSERT(imap.br_startblock != DELAYSTARTBLOCK); > > The change in code flow makes this assert rather useless, I think, since > we only end up in this branch if we have a write and a hole. If the > condition that it checks is important (and it seems to be?) then it > ought to be hoisted up a level and turned into: > > ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK); > > Right? Actually even for !write we should not see delalloc blocks here. So I'll fix up the assert in a separate prep patch.
On Tue, Oct 29, 2019 at 08:58:43AM +0100, Christoph Hellwig wrote: > On Mon, Oct 28, 2019 at 09:12:45AM -0700, Darrick J. Wong wrote: > > On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote: > > > Even if we are asked for a write layout there is no point in logging > > > the inode unless we actually modified it in some way. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------ > > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c > > > index 9c96493be9e0..fa90c6334c7c 100644 > > > --- a/fs/xfs/xfs_pnfs.c > > > +++ b/fs/xfs/xfs_pnfs.c > > > @@ -147,32 +147,27 @@ xfs_fs_map_blocks( > > > if (error) > > > goto out_unlock; > > > > > > - if (write) { > > > - enum xfs_prealloc_flags flags = 0; > > > - > > > + if (write && > > > + (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) { > > > ASSERT(imap.br_startblock != DELAYSTARTBLOCK); > > > > The change in code flow makes this assert rather useless, I think, since > > we only end up in this branch if we have a write and a hole. If the > > condition that it checks is important (and it seems to be?) then it > > ought to be hoisted up a level and turned into: > > > > ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK); > > > > Right? > > Actually even for !write we should not see delalloc blocks here. > So I'll fix up the assert in a separate prep patch. <shrug> I could just fix it, unless you're about to resend the whole series? --D
On Wed, Oct 30, 2019 at 09:12:48AM -0700, Darrick J. Wong wrote: > > Actually even for !write we should not see delalloc blocks here. > > So I'll fix up the assert in a separate prep patch. > > <shrug> I could just fix it, unless you're about to resend the whole series? I have the series ready to resend.
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c index 9c96493be9e0..fa90c6334c7c 100644 --- a/fs/xfs/xfs_pnfs.c +++ b/fs/xfs/xfs_pnfs.c @@ -147,32 +147,27 @@ xfs_fs_map_blocks( if (error) goto out_unlock; - if (write) { - enum xfs_prealloc_flags flags = 0; - + if (write && + (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) { ASSERT(imap.br_startblock != DELAYSTARTBLOCK); - if (!nimaps || imap.br_startblock == HOLESTARTBLOCK) { - /* - * xfs_iomap_write_direct() expects to take ownership of - * the shared ilock. - */ - xfs_ilock(ip, XFS_ILOCK_SHARED); - error = xfs_iomap_write_direct(ip, offset, length, - &imap, nimaps); - if (error) - goto out_unlock; - - /* - * Ensure the next transaction is committed - * synchronously so that the blocks allocated and - * handed out to the client are guaranteed to be - * present even after a server crash. - */ - flags |= XFS_PREALLOC_SET | XFS_PREALLOC_SYNC; - } - - error = xfs_update_prealloc_flags(ip, flags); + /* + * xfs_iomap_write_direct() expects to take ownership of the + * shared ilock. + */ + xfs_ilock(ip, XFS_ILOCK_SHARED); + error = xfs_iomap_write_direct(ip, offset, length, &imap, + nimaps); + if (error) + goto out_unlock; + + /* + * Ensure the next transaction is committed synchronously so + * that the blocks allocated and handed out to the client are + * guaranteed to be present even after a server crash. + */ + error = xfs_update_prealloc_flags(ip, + XFS_PREALLOC_SET | XFS_PREALLOC_SYNC); if (error) goto out_unlock; }
Even if we are asked for a write layout there is no point in logging the inode unless we actually modified it in some way. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-)