Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: Satya Tangirala <satyat@google.com>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-xfs@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [f2fs-dev] [PATCH v6 1/7] fscrypt: Add functions for direct I/O support
Date: Mon, 27 Jul 2020 10:58:48 +1000
Message-ID: <20200727005848.GV2005@dread.disaster.area> (raw)
In-Reply-To: <20200726024920.GB14321@sol.localdomain>

On Sat, Jul 25, 2020 at 07:49:20PM -0700, Eric Biggers wrote:
> On Sat, Jul 25, 2020 at 10:14:41AM +1000, Dave Chinner wrote:
> > > +bool fscrypt_dio_supported(struct kiocb *iocb, struct iov_iter *iter)
> > > +{
> > > +	const struct inode *inode = file_inode(iocb->ki_filp);
> > > +	const unsigned int blocksize = i_blocksize(inode);
> > > +
> > > +	/* If the file is unencrypted, no veto from us. */
> > > +	if (!fscrypt_needs_contents_encryption(inode))
> > > +		return true;
> > > +
> > > +	/* We only support direct I/O with inline crypto, not fs-layer crypto */
> > > +	if (!fscrypt_inode_uses_inline_crypto(inode))
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * Since the granularity of encryption is filesystem blocks, the I/O
> > > +	 * must be block aligned -- not just disk sector aligned.
> > > +	 */
> > > +	if (!IS_ALIGNED(iocb->ki_pos | iov_iter_alignment(iter), blocksize))
> > > +		return false;
> > 
> > Doesn't this force user buffers to be filesystem block size aligned,
> > instead of 512 byte aligned as is typical for direct IO?
> > 
> > That's going to cause applications that work fine on normal
> > filesystems becaues the memalign() buffers to 512 bytes or logical
> > block device sector sizes (as per the open(2) man page) to fail on
> > encrypted volumes, and it's not going to be obvious to users as to
> > why this happens.
> 
> The status quo is that direct I/O on encrypted files falls back to buffered I/O.

Largely irrelevant.

You claimed in another thread that performance is a key feature that
inline encryption + DIO provides. Now you're implying that failing
to provide that performance doesn't really matter at all.

> So this patch is strictly an improvement; it's making direct I/O work in a case
> where previously it didn't work.

Improvements still need to follow longstanding conventions. And,
IMO, it's not an improvement if the feature results in 
unpredictable performance for userspace applications.

i.e. there is no point in enabling direct IO if it is unpredictably
going to fall back to the buffered IO path when applications are
coded to the guidelines the man page said they should use. Such
problems are an utter PITA to diagnose in the field, and on those
grounds alone the current implementation gets a NACK.

> Note that there are lots of other cases where ext4 and f2fs fall back to
> buffered I/O; see ext4_dio_supported() and f2fs_force_buffered_io().  So this
> isn't a new problem.

No shit, sherlock. But that's also irrelevant to the discussion at
hand - claiming "we can fall back to buffered IO" doesn't address
the problem I've raised. It's just an excuse for not fixing it.

Indeed, the problem is easy to fix - fscrypt only cares that the
user IO offset and length is DUN aligned.  fscrypt does not care
that the user memory buffer is filesystem block aligned - user
memory buffer alignment is an underlying hardware DMA constraint -
and so fscrypt_dio_supported() needs to relax or remove the user
memroy buffer alignment constraint so that it follows existing
conventions....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 18:44 [f2fs-dev] [PATCH v6 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala via Linux-f2fs-devel
2020-07-24 18:44 ` [f2fs-dev] [PATCH v6 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala via Linux-f2fs-devel
2020-07-25  0:14   ` Dave Chinner
2020-07-26  2:49     ` Eric Biggers
2020-07-27  0:58       ` Dave Chinner [this message]
2020-07-27  2:59         ` Eric Biggers
2020-07-27  4:47           ` Dave Chinner
2020-07-24 18:44 ` [f2fs-dev] [PATCH v6 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala via Linux-f2fs-devel
2020-07-24 18:44 ` [f2fs-dev] [PATCH v6 3/7] iomap: support direct I/O with " Satya Tangirala via Linux-f2fs-devel
2020-07-24 18:44 ` [f2fs-dev] [PATCH v6 4/7] ext4: " Satya Tangirala via Linux-f2fs-devel
2020-07-24 18:44 ` [f2fs-dev] [PATCH v6 5/7] f2fs: " Satya Tangirala via Linux-f2fs-devel
2020-07-24 18:45 ` [f2fs-dev] [PATCH v6 6/7] fscrypt: document inline encryption support Satya Tangirala via Linux-f2fs-devel
2020-07-27 16:43   ` Eric Biggers
2020-07-24 18:45 ` [f2fs-dev] [PATCH v6 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala via Linux-f2fs-devel

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=20200727005848.GV2005@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=ebiggers@kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=satyat@google.com \
    /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: link

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git