From: Calvin Owens <calvinowens@fb.com> To: Brian Foster <bfoster@redhat.com> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>, <linux-xfs@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Christoph Hellwig <hch@lst.de>, Dave Chinner <david@fromorbit.com>, <kernel-team@fb.com>, <stable@vger.kernel.org>, calvinowens <calvinowens@fb.com> Subject: Re: [PATCH v2] xfs: Honor FALLOC_FL_KEEP_SIZE when punching ends of files Date: Tue, 21 Mar 2017 12:13:21 -0700 [thread overview] Message-ID: <ab15bba5-601a-3297-959c-ddb04b42d082@fb.com> (raw) In-Reply-To: <20170321113932.GA58653@bfoster.bfoster> On 03/21/2017 04:39 AM, Brian Foster wrote: > On Sun, Mar 19, 2017 at 09:54:51PM -0700, Calvin Owens wrote: >> When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will >> round the file size up to the nearest multiple of PAGE_SIZE: >> >> calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1 >> calvinow@vm-disks/generic-xfs-1 ~$ stat test >> Size: 2048 Blocks: 8 IO Block: 4096 regular file >> calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test >> calvinow@vm-disks/generic-xfs-1 ~$ stat test >> Size: 4096 Blocks: 8 IO Block: 4096 regular file >> >> Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced >> xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers >> don't enforce that [pos,offset) lies strictly on [0,i_size) when being >> called from xfs_free_file_space(), so by "leaking" these ranges into >> xfs_zero_range() we get this buggy behavior. >> >> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did >> against i_size at the bottom of xfs_free_file_space(). >> >> Reported-by: Aaron Gao <gzh@fb.com> >> Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: <stable@vger.kernel.org> # 4.8+ >> Signed-off-by: Calvin Owens <calvinowens@fb.com> >> --- >> fs/xfs/xfs_bmap_util.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c >> index 8b75dce..0796ebc 100644 >> --- a/fs/xfs/xfs_bmap_util.c >> +++ b/fs/xfs/xfs_bmap_util.c >> @@ -1309,6 +1309,17 @@ xfs_free_file_space( >> } >> >> /* >> + * Avoid doing I/O beyond eof - it's not necessary >> + * since nothing can read beyond eof. The space will >> + * be zeroed when the file is extended anyway. >> + */ > > I'd suggest to update the comment below with this information and move > the following bits down below it as well. Will do. >> + if (offset >= XFS_ISIZE(ip)) >> + return 0; >> + >> + if ((offset + len) >= XFS_ISIZE(ip)) >> + len = XFS_ISIZE(ip) - offset - 1; >> + > > This looks like an off-by-one. Do you mean the following? > > if (offset + len > XFS_ISIZE(ip)) > len = XFS_ISIZE(ip) - offset; It's not an off-by-one (it's self-consistent), but your way makes more sense, I'll fix it ;) Thanks, Calvin > Brian > >> + /* >> * Now that we've unmap all full blocks we'll have to zero out any >> * partial block at the beginning and/or end. xfs_zero_range is >> * smart enough to skip any holes, including those we just created. >> -- >> 2.9.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Calvin Owens <calvinowens@fb.com> To: Brian Foster <bfoster@redhat.com> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>, linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org, Christoph Hellwig <hch@lst.de>, Dave Chinner <david@fromorbit.com>, kernel-team@fb.com, stable@vger.kernel.org, calvinowens <calvinowens@fb.com> Subject: Re: [PATCH v2] xfs: Honor FALLOC_FL_KEEP_SIZE when punching ends of files Date: Tue, 21 Mar 2017 12:13:21 -0700 [thread overview] Message-ID: <ab15bba5-601a-3297-959c-ddb04b42d082@fb.com> (raw) In-Reply-To: <20170321113932.GA58653@bfoster.bfoster> On 03/21/2017 04:39 AM, Brian Foster wrote: > On Sun, Mar 19, 2017 at 09:54:51PM -0700, Calvin Owens wrote: >> When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will >> round the file size up to the nearest multiple of PAGE_SIZE: >> >> calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1 >> calvinow@vm-disks/generic-xfs-1 ~$ stat test >> Size: 2048 Blocks: 8 IO Block: 4096 regular file >> calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test >> calvinow@vm-disks/generic-xfs-1 ~$ stat test >> Size: 4096 Blocks: 8 IO Block: 4096 regular file >> >> Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced >> xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers >> don't enforce that [pos,offset) lies strictly on [0,i_size) when being >> called from xfs_free_file_space(), so by "leaking" these ranges into >> xfs_zero_range() we get this buggy behavior. >> >> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did >> against i_size at the bottom of xfs_free_file_space(). >> >> Reported-by: Aaron Gao <gzh@fb.com> >> Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: <stable@vger.kernel.org> # 4.8+ >> Signed-off-by: Calvin Owens <calvinowens@fb.com> >> --- >> fs/xfs/xfs_bmap_util.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c >> index 8b75dce..0796ebc 100644 >> --- a/fs/xfs/xfs_bmap_util.c >> +++ b/fs/xfs/xfs_bmap_util.c >> @@ -1309,6 +1309,17 @@ xfs_free_file_space( >> } >> >> /* >> + * Avoid doing I/O beyond eof - it's not necessary >> + * since nothing can read beyond eof. The space will >> + * be zeroed when the file is extended anyway. >> + */ > > I'd suggest to update the comment below with this information and move > the following bits down below it as well. Will do. >> + if (offset >= XFS_ISIZE(ip)) >> + return 0; >> + >> + if ((offset + len) >= XFS_ISIZE(ip)) >> + len = XFS_ISIZE(ip) - offset - 1; >> + > > This looks like an off-by-one. Do you mean the following? > > if (offset + len > XFS_ISIZE(ip)) > len = XFS_ISIZE(ip) - offset; It's not an off-by-one (it's self-consistent), but your way makes more sense, I'll fix it ;) Thanks, Calvin > Brian > >> + /* >> * Now that we've unmap all full blocks we'll have to zero out any >> * partial block at the beginning and/or end. xfs_zero_range is >> * smart enough to skip any holes, including those we just created. >> -- >> 2.9.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-03-21 19:13 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-18 1:24 [PATCH] xfs: Honor FALLOC_FL_KEEP_SIZE when punching ends of files Calvin Owens 2017-03-18 1:24 ` Calvin Owens 2017-03-18 2:54 ` Calvin Owens 2017-03-20 4:54 ` [PATCH v2] " Calvin Owens 2017-03-20 4:54 ` Calvin Owens 2017-03-21 11:39 ` Brian Foster 2017-03-21 19:13 ` Calvin Owens [this message] 2017-03-21 19:13 ` Calvin Owens 2017-03-31 4:18 ` [PATCH v3] " Calvin Owens 2017-03-31 4:18 ` Calvin Owens 2017-03-31 6:51 ` Christoph Hellwig 2017-04-03 17:15 ` Darrick J. Wong 2017-04-03 17:18 ` Darrick J. Wong 2017-04-03 18:54 ` Christoph Hellwig
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ab15bba5-601a-3297-959c-ddb04b42d082@fb.com \ --to=calvinowens@fb.com \ --cc=bfoster@redhat.com \ --cc=darrick.wong@oracle.com \ --cc=david@fromorbit.com \ --cc=hch@lst.de \ --cc=kernel-team@fb.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.