| Message ID | 542C1314.3030603@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Oct 01, 2014 at 09:43:32AM -0500, Eric Sandeen wrote: > > That sounds like a plan. If we only enable it automatically at mount > > time (iff we mounted the file system read/write) if any of the ext3 or > > ext4 specific features are enabled, that should be completely safe. > > Ok, so do that, and don't bump the reservations? I suppose > the size test & superblock write can be removed, then... > > This does bug me a little; at one point we were very carefully not > enabling any new features by mounting with a new kernel; that was > specific to mounting-ext2-with-ext4 etc, but it still feels slightly > inconsistent. Although I guess we enable it today by mounting-and- > writing-a-big-enough-file. Yeah, this behaviour was one that dates back a *long* time, before we established the rule that we don't enable any new features automatically. If this was a new feature, I wouldn't be advocating this. But if we change this now, we could introduce a regression, or at least a surprising breakage. > Something like this should fix it too, though, with less unexpected > behind-your-back behavior: > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 3aa26e9..2f94cd6 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -2563,9 +2563,15 @@ retry_grab: > * if there is delayed block allocation. But we still need > * to journalling the i_disksize update if writes to the end > * of file which has an already mapped buffer. > + * If this write might need to update the superblock due to the > + * filesize adding a new superblock feature flag, add that too. > */ > retry_journal: > - handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, 1); > + handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, > + EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb, > + EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ? > + 1 : 2); > + Yes, I suppose that would work as well. It means that file systems which don't have LARGE_FILE will waste a bit more space in the journal, causing the journal to potentially close prematurely. The code would be a bit simpler if we removed "set only if i_size has gotten too big", and replaced it with a "set it unconditionally at mount time". So there are tradeoffs with either approach. At this point I'm slightly in favor of enabling it by default if ext4 features are enabled, either in the kernel or in the e2fsck. And if we're going to do that, doing it in the kernel is more foolproof, and it will have the same net result. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/1/14 2:59 PM, Theodore Ts'o wrote: > On Wed, Oct 01, 2014 at 09:43:32AM -0500, Eric Sandeen wrote: >>> That sounds like a plan. If we only enable it automatically at mount >>> time (iff we mounted the file system read/write) if any of the ext3 or >>> ext4 specific features are enabled, that should be completely safe. >> >> Ok, so do that, and don't bump the reservations? I suppose >> the size test & superblock write can be removed, then... >> >> This does bug me a little; at one point we were very carefully not >> enabling any new features by mounting with a new kernel; that was >> specific to mounting-ext2-with-ext4 etc, but it still feels slightly >> inconsistent. Although I guess we enable it today by mounting-and- >> writing-a-big-enough-file. > > Yeah, this behaviour was one that dates back a *long* time, before we > established the rule that we don't enable any new features > automatically. If this was a new feature, I wouldn't be advocating > this. But if we change this now, we could introduce a regression, or > at least a surprising breakage. > >> Something like this should fix it too, though, with less unexpected >> behind-your-back behavior: >> >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index 3aa26e9..2f94cd6 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -2563,9 +2563,15 @@ retry_grab: >> * if there is delayed block allocation. But we still need >> * to journalling the i_disksize update if writes to the end >> * of file which has an already mapped buffer. >> + * If this write might need to update the superblock due to the >> + * filesize adding a new superblock feature flag, add that too. >> */ >> retry_journal: >> - handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, 1); >> + handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, >> + EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb, >> + EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ? >> + 1 : 2); >> + > > Yes, I suppose that would work as well. It means that file systems > which don't have LARGE_FILE will waste a bit more space in the > journal, causing the journal to potentially close prematurely. > > The code would be a bit simpler if we removed "set only if i_size has > gotten too big", and replaced it with a "set it unconditionally at > mount time". So there are tradeoffs with either approach. At this > point I'm slightly in favor of enabling it by default if ext4 features > are enabled, either in the kernel or in the e2fsck. And if we're > going to do that, doing it in the kernel is more foolproof, and it > will have the same net result. Ok. I guess this is only an issue for ext4 - well, at least this specific issue. Delalloc makes it much different than ext2 & ext3, which reserve quite a lot more. Whether there's a corner case over there which breaks, I dunno... So it seems like the simplest test is simply: Are we RW mounted with delalloc? And if so, update the feature. Seems simpler than mucking with "which features are unique to ext4" (because we could be mounting ext3-with-ext4, having no ext4-specific features, and still hit the problem right? ... test test test ... right.) I'll whip that up. Thanks, -Eric > - Ted > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 01, 2014 at 03:37:17PM -0500, Eric Sandeen wrote: > > Ok. I guess this is only an issue for ext4 - well, at least this specific > issue. Delalloc makes it much different than ext2 & ext3, which reserve quite a > lot more. Whether there's a corner case over there which breaks, I dunno... > > So it seems like the simplest test is simply: Are we RW mounted with delalloc? > And if so, update the feature. Seems simpler than mucking with "which features > are unique to ext4" I'd do "are we RW mounted with the extents feature". That way we don't need to worry about someone accidentally mounting a partition meant for Hurd using ext4, which would imply delalloc, and then causing Hurd to no longer be able to deal with the file system. That *shouldn't* happen, but if someone accidentally mounts the file system with -t ext4, but it seems safer to gate it on the existence of the extents feature. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/1/14 5:43 PM, Theodore Ts'o wrote: > On Wed, Oct 01, 2014 at 03:37:17PM -0500, Eric Sandeen wrote: >> >> Ok. I guess this is only an issue for ext4 - well, at least this specific >> issue. Delalloc makes it much different than ext2 & ext3, which reserve quite a >> lot more. Whether there's a corner case over there which breaks, I dunno... >> >> So it seems like the simplest test is simply: Are we RW mounted with delalloc? >> And if so, update the feature. Seems simpler than mucking with "which features >> are unique to ext4" > > I'd do "are we RW mounted with the extents feature". That way we > don't need to worry about someone accidentally mounting a partition > meant for Hurd using ext4, which would imply delalloc, and then > causing Hurd to no longer be able to deal with the file system. That > *shouldn't* happen, but if someone accidentally mounts the file system > with -t ext4, but it seems safer to gate it on the existence of the > extents feature. Problem is, we can hit the same problem with an ext3 filesystem (no extents) mounted with -t ext4 (enabling delalloc). Ugh. Can't we just bump the da write reservation to 2 and be done with it? ;) (AFAICT the non-delalloc reservations can be wildly overestimated). Or maybe ext4_journal_extend() when we try to update the superblock? It could fail, but it wouldn't be catastrophic if it did, fsck would find that the feature is missing... -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 02-10-14 00:49:09, Eric Sandeen wrote: > On 10/1/14 5:43 PM, Theodore Ts'o wrote: > > On Wed, Oct 01, 2014 at 03:37:17PM -0500, Eric Sandeen wrote: > >> > >> Ok. I guess this is only an issue for ext4 - well, at least this specific > >> issue. Delalloc makes it much different than ext2 & ext3, which reserve quite a > >> lot more. Whether there's a corner case over there which breaks, I dunno... > >> > >> So it seems like the simplest test is simply: Are we RW mounted with delalloc? > >> And if so, update the feature. Seems simpler than mucking with "which features > >> are unique to ext4" > > > > I'd do "are we RW mounted with the extents feature". That way we > > don't need to worry about someone accidentally mounting a partition > > meant for Hurd using ext4, which would imply delalloc, and then > > causing Hurd to no longer be able to deal with the file system. That > > *shouldn't* happen, but if someone accidentally mounts the file system > > with -t ext4, but it seems safer to gate it on the existence of the > > extents feature. > > Problem is, we can hit the same problem with an ext3 filesystem (no > extents) mounted with -t ext4 (enabling delalloc). > > Ugh. Can't we just bump the da write reservation to 2 and be done with it? ;) > (AFAICT the non-delalloc reservations can be wildly overestimated). > > Or maybe ext4_journal_extend() when we try to update the superblock? > It could fail, but it wouldn't be catastrophic if it did, fsck would find > that the feature is missing... A couple of notes: 1) Using 2 would be fine. Journal code is clever enough and it returns unused handle credits to the transaction so using 2 instead of 1 limits only the number of handles in ext4_da_write_begin() running in parallel. So I'd frankly just bump the number to 2 (with a comment!) and be done with it. 2) If we want to optimize a bit, we can check whether the write is going to extend beyond 2G and first set the feature in a separate transaction. Honza
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 3aa26e9..2f94cd6 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -2563,9 +2563,15 @@ retry_grab: * if there is delayed block allocation. But we still need * to journalling the i_disksize update if writes to the end * of file which has an already mapped buffer. + * If this write might need to update the superblock due to the + * filesize adding a new superblock feature flag, add that too. */ retry_journal: - handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, 1); + handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, + EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb, + EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ? + 1 : 2); + if (IS_ERR(handle)) { page_cache_release(page); return PTR_ERR(handle);