On Fri, 22 Oct 2010, Ric Wheeler wrote: > On 10/22/2010 07:43 AM, Lukas Czerner wrote: > > On Fri, 22 Oct 2010, Ric Wheeler wrote: > > > > > On 10/22/2010 05:12 AM, Lukas Czerner wrote: > > > > On Thu, 21 Oct 2010, Andreas Dilger wrote: > > > > > > > > > On 2010-10-21, at 08:15, Lukas Czerner wrote: > > > > > > In Pass 5 when we are checking block and inode bitmaps we have great > > > > > > opportunity to discard free space and unused inodes on the device, > > > > > > because bitmaps has just been verified as valid. This commit takes > > > > > > advantage of this opportunity and discards both, all free space and > > > > > > unused inodes. > > > > > > > > > > > > I have added new option '-K' which when set, disables discard. Also > > > > > > when > > > > > > the underlying device does not support discard, or BLKDISCARD ioctl > > > > > > returns any kind of error, or when some errors occurred in bitmaps, > > > > > > the > > > > > > discard is disabled. > > > > > I'm always a bit nervous with patches like this, that will prevent > > > > > data > > > > > recovery after an e2fsck run (which seems like the opposite of what we > > > > > want from e2fsck). > > > > > > > > > > Two suggestions: > > > > > - it probably makes sense to disable this by default, and allow it to > > > > > be > > > > > specified on the command-line and e2fsck.conf > > > > > - should we really have a short option, or a "-E discard" and "-E > > > > > nodiscard" > > > > > options, which allow us to change the default easily at some later > > > > > time > > > > > (which we can't do with a single -K flag) > > > > Right, I agree it would be probably better to disable this by default. > > > > > > > > > > > If we do disable it by default, I think that we might also want to be > > > consistent and disable the discard support in mkfs by default as well? > > > > > > thanks! > > > > > > Ric > > > > > I think that this will not be necessary. There is a concern that it might > > prevent data recovery after fsck because it might be already discarded > > (some weird fs corruption?) in pass 5. However in my opinion this is a > > very small window (if there even is any), because we have already passed > > check 1-4 and we have just confirmed that group descriptors should be ok. > > But when there is an even slight chance this might happen I would suggest > > that we really disable it by default (at least for a while - we will see > > then). > > > > On the other hand there is nothing to be afraid of in the case of mkfs, > > because we can not possibly lose any relevant data, because discard is > > done before the filesystem gets created. > > > > -Lukas > > My concern with mkfs is that we have seen several devices which don't handle > this well. > > We will be using this TRIM (or UNMAP, etc) on lots of old, creaky hardware > with old firmware, so having it try on all devices is almost certainly going > to cause breakages, hangs, etc in the field.... > > Ric > Well, so far the only breakages I have seen was with lots of small TRIMs (or UNMAPs, etc) issued in random pattern, never in case of mkfs which is quite a opposite - big sequential ranges. Hangs should be covered by those two patches: http://marc.info/?l=linux-ext4&m=128774558623608&w=2 http://marc.info/?l=linux-ext4&m=128767099123375&w=2 if, of course, they get upstream. Also there is a big win, when discard also zeroes data, because in that case we can just skip inode table initialization (zeroing) without any need of in-kernel lazyinit code enabled. And we get all this for free. It was introduced with Sandeens patch: http://marc.info/?l=linux-ext4&m=128234048208327&w=2 So, I would rather leave it on by default. -Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
