| Message ID | 20200615160244.741244-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap: Make sure iomap_end is called after iomap_begin | expand |
On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote: > Make sure iomap_end is always called when iomap_begin succeeds: the > filesystem may take locks in iomap_begin and release them in iomap_end, > for example. Ok, i get that from the patch, but I don't know anything else about this problem, and nor will anyone else trying to determine if this is a fix they need to backport to other kernels. Can you add some more information to the commit message, such as how was this found and what filesystems it affects? It would also be good to know what commit introduced this issue and whether it need stable back ports (i.e. a Fixes tag). Cheers, Dave.
On Tue, Jun 16, 2020 at 09:32:39AM +1000, Dave Chinner wrote: > On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote: > > Make sure iomap_end is always called when iomap_begin succeeds: the > > filesystem may take locks in iomap_begin and release them in iomap_end, > > for example. > > Ok, i get that from the patch, but I don't know anything else about > this problem, and nor will anyone else trying to determine if this > is a fix they need to backport to other kernels. Can you add some > more information to the commit message, such as how was this found > and what filesystems it affects? It would also be good to know what > commit introduced this issue and whether it need stable back ports > (i.e. a Fixes tag). I'd assume Andreas is looking at converting a filesystem to use iomap, since this problem only occurs for filesystems which have returned an invalid extent. I almost wonder if this should return -EFSCORRUPTED rather than -EIO. Um, except that's currently a per-fs define. Is it time to move that up to errno.h?
On Mon, Jun 15, 2020 at 04:44:37PM -0700, Matthew Wilcox wrote: > On Tue, Jun 16, 2020 at 09:32:39AM +1000, Dave Chinner wrote: > > On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote: > > > Make sure iomap_end is always called when iomap_begin succeeds: the > > > filesystem may take locks in iomap_begin and release them in iomap_end, > > > for example. > > > > Ok, i get that from the patch, but I don't know anything else about > > this problem, and nor will anyone else trying to determine if this > > is a fix they need to backport to other kernels. Can you add some > > more information to the commit message, such as how was this found > > and what filesystems it affects? It would also be good to know what > > commit introduced this issue and whether it need stable back ports > > (i.e. a Fixes tag). > > I'd assume Andreas is looking at converting a filesystem to use iomap, > since this problem only occurs for filesystems which have returned an > invalid extent. Well, I can assume it's gfs2, but you know what happens when you assume something.... > I almost wonder if this should return -EFSCORRUPTED rather than -EIO. We've typically used -EFSCORRUPTED for metadata corruptions and -EIO for data IO errors that result from metadata corruption. -EIO implies that the IO failed and the state of the data is indeterminate (i.e. may be corrupted), but the filesystem is still ok, whereas EFSCORRUPTED implies the filesystem needs to have fsck run on it to diagnose and fix the metadata corruption. This code sort of spans the grey area between them. The error could come from in in-memory bug and not actually a filesystem corruption, so it's not clear that corruption is the right thing to report here... > Um, except that's currently a per-fs define. Is it time to move that > up to errno.h? Has the "EFSCORRUPTED = EUCLEAN is stupid - let's break a longstanding user API by defining it to something completely new" yelling died down enough in the 6 months since this was last proposed? https://lore.kernel.org/linux-xfs/20191031010736.113783-1-Valdis.Kletnieks@vt.edu/ https://lore.kernel.org/linux-xfs/20191104014510.102356-11-Valdis.Kletnieks@vt.edu/ We've only been trying to get a generic -EFSCORRUPTED definition into the kernel errno headers for ~15 years... :( Cheers, Dave.
----- Original Message ----- > > I'd assume Andreas is looking at converting a filesystem to use iomap, > > since this problem only occurs for filesystems which have returned an > > invalid extent. > > Well, I can assume it's gfs2, but you know what happens when you > assume something.... Yes, it's gfs2, which already has iomap. I found the bug while just browsing the code: gfs2 takes a lock in the begin code. If there's an error, however unlikely, the end code is never called, so we would never unlock. It doesn't matter to me whether the error is -EIO because it's very unlikely in the first place. I haven't looked back to see where the problem was introduced, but I suspect it should be ported back to stable releases. Bob Peterson
On Tue, Jun 16, 2020 at 08:17:28AM -0400, Bob Peterson wrote: > ----- Original Message ----- > > > I'd assume Andreas is looking at converting a filesystem to use iomap, > > > since this problem only occurs for filesystems which have returned an > > > invalid extent. > > > > Well, I can assume it's gfs2, but you know what happens when you > > assume something.... > > Yes, it's gfs2, which already has iomap. I found the bug while just browsing > the code: gfs2 takes a lock in the begin code. If there's an error, > however unlikely, the end code is never called, so we would never unlock. > It doesn't matter to me whether the error is -EIO because it's very unlikely > in the first place. I haven't looked back to see where the problem was > introduced, but I suspect it should be ported back to stable releases. It shouldn't just be "unlikely", it should be impossible. This is the iomap code checking whether you've returned an extent which doesn't cover the range asked for. I don't think it needs to be backported, and I'm pretty neutral on whether it needs to be applied.
On Tue, Jun 16, 2020 at 3:23 PM Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Jun 16, 2020 at 08:17:28AM -0400, Bob Peterson wrote: > > ----- Original Message ----- > > > > I'd assume Andreas is looking at converting a filesystem to use iomap, > > > > since this problem only occurs for filesystems which have returned an > > > > invalid extent. > > > > > > Well, I can assume it's gfs2, but you know what happens when you > > > assume something.... > > > > Yes, it's gfs2, which already has iomap. I found the bug while just browsing > > the code: gfs2 takes a lock in the begin code. If there's an error, > > however unlikely, the end code is never called, so we would never unlock. > > It doesn't matter to me whether the error is -EIO because it's very unlikely > > in the first place. I haven't looked back to see where the problem was > > introduced, but I suspect it should be ported back to stable releases. > > It shouldn't just be "unlikely", it should be impossible. This is the > iomap code checking whether you've returned an extent which doesn't cover > the range asked for. I don't think it needs to be backported, and I'm > pretty neutral on whether it needs to be applied. Right, when these warnings trigger, the filesystem has already screwed up; this fix only makes things less bad. Those kinds of issues are very likely to be fixed long before the code hits users, so it shouldn't be backported. This bug was in iomap_apply right from the start, so: Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure") Thanks, Andreas
On Tue, Jun 16, 2020 at 03:57:08PM +0200, Andreas Gruenbacher wrote: > On Tue, Jun 16, 2020 at 3:23 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Jun 16, 2020 at 08:17:28AM -0400, Bob Peterson wrote: > > > ----- Original Message ----- > > > > > I'd assume Andreas is looking at converting a filesystem to use iomap, > > > > > since this problem only occurs for filesystems which have returned an > > > > > invalid extent. > > > > > > > > Well, I can assume it's gfs2, but you know what happens when you > > > > assume something.... > > > > > > Yes, it's gfs2, which already has iomap. I found the bug while just browsing > > > the code: gfs2 takes a lock in the begin code. If there's an error, > > > however unlikely, the end code is never called, so we would never unlock. > > > It doesn't matter to me whether the error is -EIO because it's very unlikely > > > in the first place. I haven't looked back to see where the problem was > > > introduced, but I suspect it should be ported back to stable releases. > > > > It shouldn't just be "unlikely", it should be impossible. This is the > > iomap code checking whether you've returned an extent which doesn't cover > > the range asked for. I don't think it needs to be backported, and I'm > > pretty neutral on whether it needs to be applied. > > Right, when these warnings trigger, the filesystem has already screwed > up; this fix only makes things less bad. Those kinds of issues are > very likely to be fixed long before the code hits users, so it > shouldn't be backported. > > This bug was in iomap_apply right from the start, so: > > Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure") So... you found this through code inspection, and not because you actually hit this on gfs2, or any of the other iomap users? I generally think this looks ok, but I want to know if I should be looking deeper. :) --D > Thanks, > Andreas >
Am Di., 16. Juni 2020 um 18:26 Uhr schrieb Darrick J. Wong <darrick.wong@oracle.com>: > On Tue, Jun 16, 2020 at 03:57:08PM +0200, Andreas Gruenbacher wrote: > > On Tue, Jun 16, 2020 at 3:23 PM Matthew Wilcox <willy@infradead.org> wrote: > > > On Tue, Jun 16, 2020 at 08:17:28AM -0400, Bob Peterson wrote: > > > > ----- Original Message ----- > > > > > > I'd assume Andreas is looking at converting a filesystem to use iomap, > > > > > > since this problem only occurs for filesystems which have returned an > > > > > > invalid extent. > > > > > > > > > > Well, I can assume it's gfs2, but you know what happens when you > > > > > assume something.... > > > > > > > > Yes, it's gfs2, which already has iomap. I found the bug while just browsing > > > > the code: gfs2 takes a lock in the begin code. If there's an error, > > > > however unlikely, the end code is never called, so we would never unlock. > > > > It doesn't matter to me whether the error is -EIO because it's very unlikely > > > > in the first place. I haven't looked back to see where the problem was > > > > introduced, but I suspect it should be ported back to stable releases. > > > > > > It shouldn't just be "unlikely", it should be impossible. This is the > > > iomap code checking whether you've returned an extent which doesn't cover > > > the range asked for. I don't think it needs to be backported, and I'm > > > pretty neutral on whether it needs to be applied. > > > > Right, when these warnings trigger, the filesystem has already screwed > > up; this fix only makes things less bad. Those kinds of issues are > > very likely to be fixed long before the code hits users, so it > > shouldn't be backported. > > > > This bug was in iomap_apply right from the start, so: > > > > Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure") > > So... you found this through code inspection, and not because you > actually hit this on gfs2, or any of the other iomap users? Bob did, yes. I've only hit those warnings in the very early stages of gfs2 iomap development, long before that code was even posted for review. > I generally think this looks ok, but I want to know if I should be > looking deeper. :) It's really supposed to be a simple, straight forward fix only. Thanks, Andreas
----- Original Message ----- > So... you found this through code inspection, and not because you > actually hit this on gfs2, or any of the other iomap users? > > I generally think this looks ok, but I want to know if I should be > looking deeper. :) > > --D Correct. Code-inspection only. I never actually hit the problem. If those errors are really so unusual and catastrophic, perhaps we should just change them to BUG_ONs or something instead. Why bother unlocking if we're already 1.9 meters underground? Bob Peterson
On Tue, Jun 16, 2020 at 12:38:38PM -0400, Bob Peterson wrote: > ----- Original Message ----- > > So... you found this through code inspection, and not because you > > actually hit this on gfs2, or any of the other iomap users? > > > > I generally think this looks ok, but I want to know if I should be > > looking deeper. :) > > > > --D > > Correct. Code-inspection only. I never actually hit the problem. > If those errors are really so unusual and catastrophic, perhaps > we should just change them to BUG_ONs or something instead. We do not panic a machine because a detectable data or filesystem corruption event has occurred. We have a viable error path to tell userspace a fatal IO error occurred so that is all the generic infrastructure should be doing. If a loud warning needs to be issued, then WARN_ON_ONCE() may be appropriate, though I suspect even that is overkill for this situation.... > Why bother unlocking if we're already 1.9 meters underground? Because then a filesystem that has shutdown because it has recognised that it is walking dead can be unmounted and the user can then run an autopsy to find and fix the problem without having to reboot the machine.... Cheers, Dave.
On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote: > Make sure iomap_end is always called when iomap_begin succeeds: the > filesystem may take locks in iomap_begin and release them in iomap_end, > for example. > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/iomap/apply.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c > index 76925b40b5fd..c00a14d825db 100644 > --- a/fs/iomap/apply.c > +++ b/fs/iomap/apply.c > @@ -46,10 +46,10 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, > ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap); > if (ret) > return ret; > - if (WARN_ON(iomap.offset > pos)) > - return -EIO; > - if (WARN_ON(iomap.length == 0)) > - return -EIO; > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { <urk> Forgot to actually review the original patch. :P Why combine these WARN_ON? Before, you could distinguish between your iomap_begin method returning zero length vs. bad offset. --D > + written = -EIO; > + goto out; > + } > > trace_iomap_apply_dstmap(inode, &iomap); > if (srcmap.type != IOMAP_HOLE) > @@ -80,6 +80,7 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, > written = actor(inode, pos, length, data, &iomap, > srcmap.type != IOMAP_HOLE ? &srcmap : &iomap); > > +out: > /* > * Now the data has been copied, commit the range we've copied. This > * should not fail unless the filesystem has had a fatal error. > > base-commit: 97e0204907ac4c42c6e94ef466a047523f34b853 > -- > 2.26.2 >
On Thu, Jun 18, 2020 at 3:39 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Mon, Jun 15, 2020 at 06:02:44PM +0200, Andreas Gruenbacher wrote: > > Make sure iomap_end is always called when iomap_begin succeeds: the > > filesystem may take locks in iomap_begin and release them in iomap_end, > > for example. > > > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > > --- > > fs/iomap/apply.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c > > index 76925b40b5fd..c00a14d825db 100644 > > --- a/fs/iomap/apply.c > > +++ b/fs/iomap/apply.c > > @@ -46,10 +46,10 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, > > ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap); > > if (ret) > > return ret; > > - if (WARN_ON(iomap.offset > pos)) > > - return -EIO; > > - if (WARN_ON(iomap.length == 0)) > > - return -EIO; > > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { > > <urk> Forgot to actually review the original patch. :P > > Why combine these WARN_ON? Before, you could distinguish between your > iomap_begin method returning zero length vs. bad offset. Right, the WARN_ONs shouldn't both report the same line number. I'll send an update. Thanks, Andreas
On Wed, Jun 17, 2020 at 06:39:01PM -0700, Darrick J. Wong wrote: > > - if (WARN_ON(iomap.offset > pos)) > > - return -EIO; > > - if (WARN_ON(iomap.length == 0)) > > - return -EIO; > > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { > > Why combine these WARN_ON? Before, you could distinguish between your > iomap_begin method returning zero length vs. bad offset. Does it matter? They're both the same problem -- the filesystem has returned an invalid iomap. I'd go further and combine the two: if (WARN_ON(iomap.offset > pos || iomap.length == 0)) { that'll save a few bytes of .text
On Thu, Jun 18, 2020 at 2:32 PM Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Jun 17, 2020 at 06:39:01PM -0700, Darrick J. Wong wrote: > > > - if (WARN_ON(iomap.offset > pos)) > > > - return -EIO; > > > - if (WARN_ON(iomap.length == 0)) > > > - return -EIO; > > > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { > > > > Why combine these WARN_ON? Before, you could distinguish between your > > iomap_begin method returning zero length vs. bad offset. > > Does it matter? They're both the same problem -- the filesystem has > returned an invalid iomap. I'd go further and combine the two: > > if (WARN_ON(iomap.offset > pos || iomap.length == 0)) { > > that'll save a few bytes of .text That would be fine by me as well. Christoph may have wanted separate warnings for a particular reason though. Andreas
On Thu, Jun 18, 2020 at 02:37:37PM +0200, Andreas Gruenbacher wrote: > On Thu, Jun 18, 2020 at 2:32 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Jun 17, 2020 at 06:39:01PM -0700, Darrick J. Wong wrote: > > > > - if (WARN_ON(iomap.offset > pos)) > > > > - return -EIO; > > > > - if (WARN_ON(iomap.length == 0)) > > > > - return -EIO; > > > > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { > > > > > > Why combine these WARN_ON? Before, you could distinguish between your > > > iomap_begin method returning zero length vs. bad offset. > > > > Does it matter? They're both the same problem -- the filesystem has > > returned an invalid iomap. I'd go further and combine the two: > > > > if (WARN_ON(iomap.offset > pos || iomap.length == 0)) { > > > > that'll save a few bytes of .text > > That would be fine by me as well. Christoph may have wanted separate > warnings for a particular reason though. Yes. The line number in the WARN_ON will tell you which condition you if they are separate, which is really useful to diagnose what is going on.
On Thu, Jun 18, 2020 at 06:56:39AM -0700, Christoph Hellwig wrote: > On Thu, Jun 18, 2020 at 02:37:37PM +0200, Andreas Gruenbacher wrote: > > On Thu, Jun 18, 2020 at 2:32 PM Matthew Wilcox <willy@infradead.org> wrote: > > > On Wed, Jun 17, 2020 at 06:39:01PM -0700, Darrick J. Wong wrote: > > > > > - if (WARN_ON(iomap.offset > pos)) > > > > > - return -EIO; > > > > > - if (WARN_ON(iomap.length == 0)) > > > > > - return -EIO; > > > > > + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { > > > > > > > > Why combine these WARN_ON? Before, you could distinguish between your > > > > iomap_begin method returning zero length vs. bad offset. > > > > > > Does it matter? They're both the same problem -- the filesystem has > > > returned an invalid iomap. I'd go further and combine the two: > > > > > > if (WARN_ON(iomap.offset > pos || iomap.length == 0)) { > > > > > > that'll save a few bytes of .text > > > > That would be fine by me as well. Christoph may have wanted separate > > warnings for a particular reason though. > > Yes. The line number in the WARN_ON will tell you which condition > you if they are separate, which is really useful to diagnose what is > going on. Thinking about it, wouldn't the second test be better replaced with: if (WARN_ON(iomap.offset + iomap.length <= pos)) in case the filesystem returns an extent which finishes before pos? This would be a superset of the test for length being 0.
On Thu, Jun 18, 2020 at 08:15:23AM -0700, Matthew Wilcox wrote: > Thinking about it, wouldn't the second test be better replaced with: > > if (WARN_ON(iomap.offset + iomap.length <= pos)) > > in case the filesystem returns an extent which finishes before pos? > This would be a superset of the test for length being 0. The idea was to tell what is wrong. Both with the initial iomap work and later the COW support I had all kinds of weird scenarious during bringup where an obvious error has been very helpful.
diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c index 76925b40b5fd..c00a14d825db 100644 --- a/fs/iomap/apply.c +++ b/fs/iomap/apply.c @@ -46,10 +46,10 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap); if (ret) return ret; - if (WARN_ON(iomap.offset > pos)) - return -EIO; - if (WARN_ON(iomap.length == 0)) - return -EIO; + if (WARN_ON(iomap.offset > pos) || WARN_ON(iomap.length == 0)) { + written = -EIO; + goto out; + } trace_iomap_apply_dstmap(inode, &iomap); if (srcmap.type != IOMAP_HOLE) @@ -80,6 +80,7 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags, written = actor(inode, pos, length, data, &iomap, srcmap.type != IOMAP_HOLE ? &srcmap : &iomap); +out: /* * Now the data has been copied, commit the range we've copied. This * should not fail unless the filesystem has had a fatal error.
Make sure iomap_end is always called when iomap_begin succeeds: the filesystem may take locks in iomap_begin and release them in iomap_end, for example. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap/apply.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) base-commit: 97e0204907ac4c42c6e94ef466a047523f34b853