On 09/01/2021 17:03, Al Viro wrote:

On Sat, Jan 09, 2021 at 04:09:08PM +0000, Pavel Begunkov wrote:

quoted

On 06/12/2020 16:01, Pavel Begunkov wrote:

quoted

On 21/11/2020 14:37, Pavel Begunkov wrote:

quoted

The problem here is that iov_iter_is_*() helpers check types for
equality, but all iterate_* helpers do bitwise ands. This confuses
compilers, so even if some cases were handled separately with
iov_iter_is_*(), corresponding ifs in iterate*() right after are not
eliminated.

E.g. iov_iter_npages() first handles discards, but iterate_all_kinds()
still checks for discard iter type and generates unreachable code down
the line.

Ping. This one should be pretty simple

Ping please. Any doubts about this patch?

Sorry, had been buried in other crap.  I'm really not fond of the
bitmap use; if anything, I would rather turn iterate_and_advance() et.al.
into switches...

How about moving the READ/WRITE part into MSB?  Checking is just as fast
(if not faster - check for sign vs. checking bit 0).  And turn the
types into straight (dense) enum.

Didn't realise that approach before, sounds good. Most of it will be
replaced with sign jcc, and the rest will be (t >> 31) or movcc, so it
should not be of concern.

type_mask = 255;
iov_iter_type(i) { return i->type & ~type_mask; }

I hope this stuff won't add much, because the original patch completely
optimises this "&" out. I guess it'll turn into extra

xor m m
notb8 m
and m & type

Almost all iov_iter_rw() callers have the form (iov_iter_rw(iter) == READ) or
(iov_iter_rw(iter) == WRITE).  Out of 50-odd callers there are 5 nominal
exceptions:
fs/cifs/smbdirect.c:1936:                        iov_iter_rw(&msg->msg_iter));
fs/exfat/inode.c:442:   int rw = iov_iter_rw(iter);
fs/f2fs/data.c:3639:    int rw = iov_iter_rw(iter);
fs/f2fs/f2fs.h:4082:    int rw = iov_iter_rw(iter);
fs/f2fs/f2fs.h:4092:    int rw = iov_iter_rw(iter);

The first one is debugging printk
        if (iov_iter_rw(&msg->msg_iter) == WRITE) {
                /* It's a bug in upper layer to get there */
                cifs_dbg(VFS, "Invalid msg iter dir %u\n",
                         iov_iter_rw(&msg->msg_iter));
                rc = -EINVAL;
                goto out;
        }
and if you look at the condition, the quality of message is
underwhelming - "Data source msg iter passed by caller" would
be more informative.

Other 4...  exfat one is
        if (rw == WRITE) {
...
	}
...
        if (ret < 0 && (rw & WRITE))
                exfat_write_failed(mapping, size);
IOW, doing
	bool is_write = iov_iter_rw(iter) == WRITE;
would be cleaner.  f2fs.h ones are
	int rw = iov_iter_rw(iter);
	....
	if (.... && rw == WRITE ...
so they are of the same sort (assuming we want that local
variable in the first place).

f2fs/data.c is the least trivial - it includes things like
                if (!down_read_trylock(&fi->i_gc_rwsem[rw])) {
and considering the amount of other stuff done there,
I would suggest something like
	int rw = is_data_source(iter) ? WRITE : READ;

I'll dig myself from under ->d_revalidate() code review, look
through the iov_iter-related series and post review, hopefully
by tonight.

Great, thanks Al. Without it being optimised right my other patches keep
worsening iov_iter, and I obviously want to avoid that.

-- 
Pavel Begunkov