On Fri, Oct 27, 2023 at 09:53:19AM +0200, Pankaj Raghav wrote: > >> - bio = iomap_dio_alloc_bio(iter, dio, 1, REQ_OP_WRITE | REQ_SYNC | REQ_IDLE); > >> + WARN_ON_ONCE(len > (BIO_MAX_VECS * PAGE_SIZE)); > > > > How can that happen here? Max fsb size will be 64kB for the > > foreseeable future, the bio can hold 256 pages so it will have a > > minimum size capability of 1MB. > > > > I just added it as a pathological check. This will not trigger > anytime in the near future. Yeah, exactly my point - it should never happen, it's a fundamental developer stuff-up bug, not a runtime bug, and so shouldn't be checked at runtime on every bio.... > > FWIW, as a general observation, I think this is the wrong place to > > be checking that a filesystem block is larger than can be fit in a > > single bio. There's going to be problems all over the place if we > > can't do fsb sized IO in a single bio. i.e. I think this sort of > > validation should be performed during filesystem mount, not > > sporadically checked with WARN_ON() checks in random places in the > > IO path... > > > > I agree that it should be a check at a higher level. > > As iomap can be used by any filesystem, the check on FSB limitation > should be a part iomap right? I don't see any explicit document/comment > that states the iomap limitations on FSB, etc. No, it should be part of the filesystem that *uses the bio infrastrucure*. We already do that with the page cache - filesystems check at mount time that the FSB is <= PAGE_SIZE and reject the mount if this check fails because the page cache simply cannot handle this situation. This is no different: if we are going to allow FSB > PAGE_SIZE, then we need to ensure somewhere, even as a BUILD_BUG_ON(), that the max support FSB the filesystem has is smaller than what we can pack in a bio. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
