On Fri, Aug 26, 2016 at 04:33:44PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 25, 2016 at 10:37:09AM -0400, Brian Foster wrote:
> > On just skimming over this so far, I feel like this should be at least
> > two patches, possibly 3:
> >
> > - Kill xfs_bmapi_delay() and pull up associated bits to iomap().
>
> As in just a move of code to xfs_iomap.c or also merged it with a
> partіal copy of xfs_file_iomap_begin? The first is trivial, but also
> rather pointless. The second is a bit more work, still very doable
> but probably also not that useful as we're going to totally rewrite it
> again in the next step.
>
> > - Possibly separate out the part that moves iteration from the (former)
> > xfs_bmapi_delay() code up to the iomap code, if we can do so cleanly.
>
> Well, the major point is that we get rid of the iteration as there isn't
> any actual need for it.
>
... which should probably be mentioned in the commit log description. In
any event, I think it's probably helpful enough to just separate out the
prealloc rework from everything else.
> > - Refactor/rework the preallocate logic.
>
> But I guess I could do a pass that creates xfs_file_iomap_begin_delay
> as in the new version except without the prealloc changes, and then
> separate them out. I don't quite see the point, though..
They don't appear to be related, for one. Second (and IMO), it makes it
notably easier to review and deal with any potential regressions down
the road. As it is, I made a more thorough pass through the code and I
didn't notice anything that looked wrong, but I'd still prefer to see
the changes separated out to be confident I didn't miss anything.
> > I'm not necessarily against cleaning up/reworking the prealloc bits, but
> > I'm not a huge fan of open coding all of this here in the iomap
> > function. If nothing else, the indentation starts to make my eyes
> > cross... could we retain one level of abstraction here for this hunk of
> > logic that updates end_fsb?
>
> We're only having three tabs of indentation. I actually looked into
> a helper for that whole block, but we'd need to pass:
>
Yeah, I suppose the indentation itself is not so much what stands out.
Looking again, I think it's more the combination of that and the
multi-line checks we're combining.
> ip, idx, prev, offset_fsb, offset, count, maxbytes_fsb
>
> (we could potentially re-derive offset_fsb from offset if we don't
> mind the inefficieny and recalculate maxbytes_fsb. This already
> assumes mp is trivially derived from ip)
>
> and return
>
> alloc_blocks, end_fsb
>
> so the function would be quite a monster in terms of its calling
> convention. Additionally we'd have the related by not qute the
> same if blocks around XFS_MOUNT_DFLT_IOSIZE and the isize split
> over two functions, which doesn't exactly help understanding
> the flow.
>
Not quite sure I follow the last bit, but I don't necessarily think the
whole thing has to be boxed into a helper to clean it up. E.g., I'd do
something like the appended diff (compile tested only).
Brian
---8<---
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 2b449f5..45e5268 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -488,6 +488,54 @@ check_writeio:
return alloc_blocks;
}
+/*
+ * If we are doing a write at the end of the file and there are no allocations
+ * past this one, then extend the allocation out to the file system's write
+ * iosize.
+ *
+ * As an exception we don't do any preallocation at all if the file is smaller
+ * than the minimum preallocation and we are using the default dynamic
+ * preallocation scheme, as it is likely this is the only write to the file that
+ * is going to be done.
+ *
+ * We clean up any extra space left over when the file is closed in
+ * xfs_inactive().
+ */
+static int
+xfs_iomap_prealloc(
+ struct xfs_inode *ip,
+ loff_t offset,
+ loff_t count,
+ xfs_extnum_t idx,
+ struct xfs_bmbt_irec *prev)
+{
+ struct xfs_mount *mp = ip->i_mount;
+ xfs_fsblock_t alloc_blocks;
+ xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset);
+
+ if (offset + count <= XFS_ISIZE(ip))
+ return 0;
+
+ if (!(mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) &&
+ (XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_writeio_blocks)))
+ return 0;
+
+ /*
+ * If an explicit allocsize is set, the file is small, or we
+ * are writing behind a hole, then use the minimum prealloc:
+ */
+ if ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
+ XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
+ idx == 0 ||
+ prev->br_startoff + prev->br_blockcount < offset_fsb)
+ alloc_blocks = mp->m_writeio_blocks;
+ else
+ alloc_blocks =
+ xfs_iomap_prealloc_size(ip, offset, prev);
+
+ return alloc_blocks;
+}
+
static int
xfs_file_iomap_begin_delay(
struct inode *inode,
@@ -507,6 +555,7 @@ xfs_file_iomap_begin_delay(
struct xfs_bmbt_irec got;
struct xfs_bmbt_irec prev;
xfs_extnum_t idx;
+ xfs_fsblock_t prealloc = 0;
ASSERT(!XFS_IS_REALTIME_INODE(ip));
ASSERT(!xfs_get_extsz_hint(ip));
@@ -554,41 +603,14 @@ xfs_file_iomap_begin_delay(
end_fsb = orig_end_fsb =
min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
- /*
- * If we are doing a write at the end of the file and there are no
- * allocations past this one, then extend the allocation out to the
- * file system's write iosize.
- *
- * As an exception we don't do any preallocation at all if the file
- * is smaller than the minimum preallocation and we are using the
- * default dynamic preallocation scheme, as it is likely this is the
- * only write to the file that is going to be done.
- *
- * We clean up any extra space left over when the file is closed in
- * xfs_inactive().
- */
- if (eof && offset + count > XFS_ISIZE(ip) &&
- ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
- XFS_ISIZE(ip) >= XFS_FSB_TO_B(mp, mp->m_writeio_blocks))) {
- xfs_fsblock_t alloc_blocks;
- xfs_off_t aligned_offset;
- xfs_extlen_t align;
-
- /*
- * If an explicit allocsize is set, the file is small, or we
- * are writing behind a hole, then use the minimum prealloc:
- */
- if ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
- XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
- idx == 0 ||
- prev.br_startoff + prev.br_blockcount < offset_fsb)
- alloc_blocks = mp->m_writeio_blocks;
- else
- alloc_blocks =
- xfs_iomap_prealloc_size(ip, offset, &prev);
+ if (eof)
+ prealloc = xfs_iomap_prealloc(ip, offset, count, idx, &prev);
+ if (prealloc) {
+ xfs_off_t aligned_offset;
+ xfs_extlen_t align;
aligned_offset = XFS_WRITEIO_ALIGN(mp, offset + count - 1);
- end_fsb = XFS_B_TO_FSBT(mp, aligned_offset) + alloc_blocks;
+ end_fsb = XFS_B_TO_FSBT(mp, aligned_offset) + prealloc;
align = xfs_eof_alignment(ip, 0);
if (align)
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
