> On Oct. 7, 2014, 5:46 p.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/arc.c, lines 4803-4804 > > <https://reviews.csiden.org/r/112/diff/1/?file=10378#file10378line4803> > > > > I see that you're keeping the meaning of these stats the same, but I > > think they were wrong before. What would you think about changing them to > > the (new, correct definition) write_asize? > > Andriy Gapon wrote: > Yes, I agree that it would be better to update these (duplicate?) stats > to the new meaning. > One issue is that they are increased in just this one place, but there > are many places where they are decreased, so vdev_psize_to_asize() would have > to be littered over a lot more code. > > Also, there could be a semi-theorical discussion about what an actual > size of a write is and what the used space actuall is. For example, let > ashift be 12 (for 4KB physical sector size) and let's assume that we write a > 1KB buffer (zio->io_size = 1024) and then skip 3KB so that the next buffer > would be written at an aligned offset. A disk will have to do > read-modify-write, so internally it writes 4KB, but for the OS the write is > 1KB. And should we treat that skipped 3KB remainder of a physical block as a > used space or as a some sort of a hole? > > But then there is this issue https://www.illumos.org/issues/5220. I think > that if that issue is fixed then the stats will automatically become correct > and we will not actually need vdev_psize_to_asize() in l2arc_write_buffers() > as all write sizes will be ashift aligned. The bug is about disks that can > not emulate 512B sectors, however I think that it should make things less > confusing for 512e kind of disks as well.
I agree that the fix described for bug 5220 would be the complete solution. But I'm OK with this fix as it stands. - Matthew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/112/#review275 ----------------------------------------------------------- On Oct. 8, 2014, 1:57 p.m., Andriy Gapon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/112/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2014, 1:57 p.m.) > > > Review request for OpenZFS Developer Mailing List, Matthew Ahrens and Saso > Kiselkov. > > > Repository: illumos-gate > > > Description > ------- > > If we don't account for that, then we might end up overwriting disk > area of buffers that have not been evicted yet, because l2arc_evict > operates in terms of disk addresses. > > The discrepancy between the write size calculation and the actual increment > to l2ad_hand was introduced in > commit e14bb3258d05c1b1077e2db7cf77088924e56919 > > Also, consistently use asize / a_sz for the allocated size, psize / p_sz > for the physical size. Where the latter accounts for possible size > reduction because of compression, whereas the former accounts for possible > size expansion because of alignment requirements. > > The code still assumes that either underlying storage subsystems or > hardware is able to do read-modify-write when an L2ARC buffer size is > not a multiple of a disk's block size. This is true for 4KB sector disks > that provide 512B sector emulation, but may not be true in general. > In other words, we currently do not have any code to make sure that > an L2ARC buffer, whether compressed or not, which is used for physical I/O > has a suitable size. > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/arc.c 69d16af3b669458e3937fe0c6a4d91755bc6e2a7 > > Diff: https://reviews.csiden.org/r/112/diff/ > > > Testing > ------- > > > Thanks, > > Andriy Gapon > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
