Re: [PATCH] libext2fs: revert "try to always use PUNCH_HOLE for unix_discard"

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Feb 15, 2019 at 10:50:07AM +0100, Lukas Czerner wrote:
> On Thu, Feb 14, 2019 at 04:04:48PM -0500, Theodore Y. Ts'o wrote:
> > On Mon, Jan 14, 2019 at 03:37:08PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Revert bcca9876a3428c10417c660b78933e6e70e8a5f5, because
> > > fallocate(PUNCH_HOLE) on block devices was changed to use zeroout
> > > instead of discard shortly after block device fallocate was merged.
> > > zeroout isn't necessarily a "drop storage" operation like discard is,
> > > so we prefer to use that on block devices.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Thanks, applied.
> > 
> > 					- Ted
> 
> I just noticed this patch, sorry. I think we can still use fallocate,
> but we need to set the right flags to make sure it uses discard instead
> of zeroout. See fs/block_dev.c
> 
> 	switch (mode) {
> 	case FALLOC_FL_ZERO_RANGE:
> 	case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE:
> 		error = blkdev_issue_zeroout(bdev, start >> 9, len >> 9,
> 					    GFP_KERNEL, BLKDEV_ZERO_NOUNMAP);
> 		break;
> 	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
> 		error = blkdev_issue_zeroout(bdev, start >> 9, len >> 9,
> 					     GFP_KERNEL, BLKDEV_ZERO_NOFALLBACK);
> 		break;
> 	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
> 		error = blkdev_issue_discard(bdev, start >> 9, len >> 9,
> 					     GFP_KERNEL, 0);
> 		break;
> 	default:
> 		return -EOPNOTSUPP;
> 	}
> 
> So if we want a discard (meaning we want to unallocate the blocks
> without necessarily making sure we can't read stale data from it) we
> have to use FALLOC_FL_NO_HIDE_STALE.
> 
> So the flags would be FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE

Userspace isn't allowed to pass in _NO_HIDE_STALE; see
FALLOC_FL_SUPPORTED_MASK in include/linux/falloc.h.

The behavior of "no hide stale" isn't defined in the manpages; it's
merely a reserved code point.

--D

> Ted, Darrick what do you think ? Can we keep the
> bcca9876a3428c10417c660b78933e6e70e8a5f5 commit and just change the
> flags ?
> 
> -Lukas