LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Gao Xiang <hsiangkao@linux.alibaba.com>
To: Chao Yu <chao@kernel.org>
Cc: linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
	nvdimm@lists.linux.dev, LKML <linux-kernel@vger.kernel.org>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Liu Bo <bo.liu@linux.alibaba.com>,
	Joseph Qi <joseph.qi@linux.alibaba.com>,
	Liu Jiang <gerry@linux.alibaba.com>,
	Huang Jianan <huangjianan@oppo.com>, Tao Ma <boyu.mt@taobao.com>
Subject: Re: [PATCH v2 1/3] erofs: iomap support for non-tailpacking DIO
Date: Wed, 4 Aug 2021 12:52:29 +0800	[thread overview]
Message-ID: <YQodDac58Uta4ZtR@B-P7TQMD6M-0146.local> (raw)
In-Reply-To: <YQoX62zRERGX9BGB@B-P7TQMD6M-0146.local>

On Wed, Aug 04, 2021 at 12:30:35PM +0800, Gao Xiang wrote:
> Hi Chao,
> 
> On Wed, Aug 04, 2021 at 10:57:08AM +0800, Chao Yu wrote:
> > On 2021/7/31 3:46, Gao Xiang wrote:
> 
> ...
> 
> > >   }
> > > +static int erofs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> > > +		unsigned int flags, struct iomap *iomap, struct iomap *srcmap)
> > > +{
> > > +	int ret;
> > > +	struct erofs_map_blocks map;
> > > +
> > > +	map.m_la = offset;
> > > +	map.m_llen = length;
> > > +
> > > +	ret = erofs_map_blocks_flatmode(inode, &map, EROFS_GET_BLOCKS_RAW);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	iomap->bdev = inode->i_sb->s_bdev;
> > > +	iomap->offset = map.m_la;
> > > +	iomap->length = map.m_llen;
> > > +	iomap->flags = 0;
> > > +
> > > +	if (!(map.m_flags & EROFS_MAP_MAPPED)) {
> > > +		iomap->type = IOMAP_HOLE;
> > > +		iomap->addr = IOMAP_NULL_ADDR;
> > > +		if (!iomap->length)
> > > +			iomap->length = length;
> > 
> > This only happens for the case offset exceeds isize?
> 
> Thanks for the review.
> 
> Yeah, this is a convention (length 0 with !EROFS_MAP_MAPPED) for post-EOF
> in erofs_map_blocks_flatmode(), need to follow iomap rule as well.
> 
> > 
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* that shouldn't happen for now */
> > > +	if (map.m_flags & EROFS_MAP_META) {
> > > +		DBG_BUGON(1);
> > > +		return -ENOTBLK;
> > > +	}
> > > +	iomap->type = IOMAP_MAPPED;
> > > +	iomap->addr = map.m_pa;
> > > +	return 0;
> > > +}
> > > +
> > > +const struct iomap_ops erofs_iomap_ops = {
> > > +	.iomap_begin = erofs_iomap_begin,
> > > +};
> > > +
> > > +static int erofs_prepare_dio(struct kiocb *iocb, struct iov_iter *to)
> > > +{
> > > +	struct inode *inode = file_inode(iocb->ki_filp);
> > > +	loff_t align = iocb->ki_pos | iov_iter_count(to) |
> > > +		iov_iter_alignment(to);
> > > +	struct block_device *bdev = inode->i_sb->s_bdev;
> > > +	unsigned int blksize_mask;
> > > +
> > > +	if (bdev)
> > > +		blksize_mask = (1 << ilog2(bdev_logical_block_size(bdev))) - 1;
> > > +	else
> > > +		blksize_mask = (1 << inode->i_blkbits) - 1;
> > > +
> > > +	if (align & blksize_mask)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Temporarily fall back tail-packing inline to buffered I/O instead
> > > +	 * since tail-packing inline support relies on an iomap core update.
> > > +	 */
> > > +	if (EROFS_I(inode)->datalayout == EROFS_INODE_FLAT_INLINE &&
> > > +	    iocb->ki_pos + iov_iter_count(to) >
> > > +			rounddown(inode->i_size, EROFS_BLKSIZ))
> > > +		return 1;
> > > +	return 0;
> > > +}
> > > +
> > > +static ssize_t erofs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > > +{
> > > +	/* no need taking (shared) inode lock since it's a ro filesystem */
> > > +	if (!iov_iter_count(to))
> > > +		return 0;
> > > +
> > > +	if (iocb->ki_flags & IOCB_DIRECT) {
> > > +		int err = erofs_prepare_dio(iocb, to);
> > > +
> > > +		if (!err)
> > > +			return iomap_dio_rw(iocb, to, &erofs_iomap_ops,
> > > +					    NULL, 0);
> > > +		if (err < 0)
> > > +			return err;
> > > +		/*
> > > +		 * Fallback to buffered I/O if the operation being performed on
> > > +		 * the inode is not supported by direct I/O. The IOCB_DIRECT
> > > +		 * flag needs to be cleared here in order to ensure that the
> > > +		 * direct I/O path within generic_file_read_iter() is not
> > > +		 * taken.
> > > +		 */
> > > +		iocb->ki_flags &= ~IOCB_DIRECT;
> > > +	}
> > > +	return generic_file_read_iter(iocb, to);
> > 
> > It looks it's fine to call filemap_read() directly since above codes have
> > covered DIO case, then we don't need to change iocb->ki_flags flag, it's
> > minor though.
> 
> Yeah, we could use filemap_read() here instead. yet IMO, it might be
> better to drop IOCB_DIRECT too to keep iocb consistent with the real
> semantics (even it's not used internally.)

After checking the other users of filemap_read(), I'm fine to leave
IOCB_DIRECT as-is. Will update.

Thanks,
Gao Xiang

  reply	other threads:[~2021-08-04  4:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 19:46 [PATCH v2 0/3] erofs: iomap support for uncompressed cases Gao Xiang
2021-07-30 19:46 ` [PATCH v2 1/3] erofs: iomap support for non-tailpacking DIO Gao Xiang
2021-08-04  2:57   ` Chao Yu
2021-08-04  4:30     ` Gao Xiang
2021-08-04  4:52       ` Gao Xiang [this message]
2021-07-30 19:46 ` [PATCH v2 2/3] erofs: dax support for non-tailpacking regular file Gao Xiang
2021-08-04  7:14   ` Chao Yu
2021-08-04 11:21     ` Gao Xiang
2021-07-30 19:46 ` [PATCH v2 3/3] erofs: convert all uncompressed cases to iomap Gao Xiang
2021-08-04  7:17   ` Chao Yu
2021-08-04 11:22     ` Gao Xiang

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=YQodDac58Uta4ZtR@B-P7TQMD6M-0146.local \
    --to=hsiangkao@linux.alibaba.com \
    --cc=bo.liu@linux.alibaba.com \
    --cc=boyu.mt@taobao.com \
    --cc=chao@kernel.org \
    --cc=djwong@kernel.org \
    --cc=gerry@linux.alibaba.com \
    --cc=huangjianan@oppo.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nvdimm@lists.linux.dev \
    --subject='Re: [PATCH v2 1/3] erofs: iomap support for non-tailpacking DIO' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).