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.
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.
