On 09/01/2021 17:03, Al Viro wrote:
> On Sat, Jan 09, 2021 at 04:09:08PM +0000, Pavel Begunkov wrote:
>> On 06/12/2020 16:01, Pavel Begunkov wrote:
>>> On 21/11/2020 14:37, Pavel Begunkov wrote:
>>>> 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
