On Tue, May 07, 2024 at 05:00:28PM +0100, Matthew Wilcox wrote:
> On Fri, May 03, 2024 at 02:53:49AM -0700, Luis Chamberlain wrote:
> > + bio = iomap_dio_alloc_bio(iter, dio, BIO_MAX_VECS,
> > + REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
> > fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> > GFP_KERNEL);
> > +
> > bio->bi_iter.bi_sector = iomap_sector(&iter->iomap, pos);
> > bio->bi_private = dio;
> > bio->bi_end_io = iomap_dio_bio_end_io;
> >
> > - __bio_add_page(bio, page, len, 0);
> > + while (len) {
> > + unsigned int io_len = min_t(unsigned int, len, PAGE_SIZE);
> > +
> > + __bio_add_page(bio, page, io_len, 0);
> > + len -= io_len;
> > + }
> > iomap_dio_submit_bio(iter, dio, bio, pos);
>
> If the len is more than PAGE_SIZE * BIO_MAX_VECS, __bio_add_page()
> will fail silently. I hate this interface.
I added a WARN_ON_ONCE() at the start of the function so that it does
not silently fail:
WARN_ON_ONCE(len > (BIO_MAX_VECS * PAGE_SIZE));
This function is used to do only sub block zeroing, and I don't think we will
cross 1MB block size in the forseeable future, and even if we do, we have
this to warn us about so that it can be changed?
>
> You should be doing something like ...
>
> while (len) {
> unsigned int io_len = min_t(unsigned int, len, PAGE_SIZE);
>
> while (!bio || bio_add_page() < io_len) {
> if (bio)
> iomap_dio_submit_bio(iter, dio, bio, pos);
> bio = iomap_dio_alloc_bio(iter, dio, BIO_MAX_VECS,
> REQ_OP_WRITE | REQ_SYNC | REQ_IDLE);
> fscrypt_set_bio_crypt_ctx(bio, inode,
> pos >> inode->i_blkbits, GFP_KERNEL);
> }
> }
--
Pankaj Raghav
