Great find, Alexander! Your diff does seem like the obvious solution. We
would also want to add a comment explaining why this case needs to be
handled specially.
My one concern with your code change is that it might be possible to change
other aspects of the object without it being freed and reallocated, in
which case we would also need to preserve the spill block. For example,
the DMU supports changing the bonus type and length of an existing object.
I'm not sure if the ZPL actually uses that functionality when also using
spill blocks, so we may be safe, but this still seems fragile. Also I
think the block size can be reduced if there is only one block. I think
the ZAP does this when transitioning from microzap to fat zap, and it may
be able to also have a spill block if you set ACL's on a directory.
There's also dmu_object_zapify() which can change the main type, but that
is only used in the MOS. We may want to change this code to do something
like:
if (main type changed || (block size changed && multiple blocks)) {
reclaim
} else {
if (bonus type changed)
dmu_set_bonustype()
if (bonus size changed)
dmu_set_bonus()
if (block size changed)
dmu_object_set_blocksize()
}
The tricky thing about the overall design is that the send stream doesn't
indicate when the spill block is not present, so we have to infer this from
other changes. If it did tell us, we could preserve the spill block by
default, even when other aspects of the dnode are changing, and only free
the spill block if the sending side explicitly tells us that it doesn't
exist on the sending side. (At least theoretically; the DMU code may make
this difficult today.)
I also looked at the ZoL code where there is encryption and large dnodes.
In that case we additionally free/claim the object if it's a raw send and
the number of levels of indirection decreased. I think that is only
possible when the object is freed and reallocated on the source side, so it
should still be OK to do the free/claim (freeing the old spill block in the
process).
--matt
On Tue, Oct 30, 2018 at 2:29 PM Alexander Motin <[email protected]> wrote:
> Hi,
>
> Last week we finally were able to reproduce and diagnose very old ZFS
> issue (checked FreeBSD, CentOS/ZoL and SmartOS), when replication code
> in receive_object() falsely assumes that if received object block size
> is different from local, then it must be a new object and calls
> dmu_object_reclaim() to wipe it out. In most cases it is not a problem,
> since all dnode, bonus buffer and data block(s) are immediately
> rewritten any way, but the problem is that spill block (if used) is not.
> This means loss of ACLs, extended attributes, etc.
>
> This issue can be triggered in very simple way:
> 1. create 4KB file with 10+ ACL entries (FreeBSD/SmartOS)
> 2. on Linux, you need to zfs set acltype=posixacl and xattr=sa
> 3. on Linux, you need to use setfattr to set extended attributes on the
> file to ensure the spill_blkptr is used
> 4. take snapshot and send snapshot to different dataset (with equal
> settings on Linux)
> 5. append another 4KB to the file
> 6. take snapshot and send incremental between first snapshot and this
> snapshot to other dataset
> 7. witness corruption (note on Linux you need to run `getfattr -d -m -
> file` to see that the extended attributes were lost)
>
> I've made an experimental patch below, trying to only change object
> block size on the receiving side if it looks like possible valid object
> grow. That fixes the problem on my tests. I can still guess scenarios
> how it could possibly be fooled, but those are much less straightforward.
>
> Any comments or better ideas?
>
> --- dmu_send.c (revision 339883)
> +++ dmu_send.c (working copy)
> @@ -2190,6 +2190,7 @@ receive_object(struct receive_writer_arg *rwa, str
>
> tx = dmu_tx_create(rwa->os);
> dmu_tx_hold_bonus(tx, object);
> + dmu_tx_hold_write(tx, object, 0, 0);
> err = dmu_tx_assign(tx, TXG_WAIT);
> if (err != 0) {
> dmu_tx_abort(tx);
> @@ -2203,7 +2204,9 @@ receive_object(struct receive_writer_arg *rwa, str
> drro->drr_bonustype, drro->drr_bonuslen,
> drro->drr_dn_slots << DNODE_SHIFT, tx);
> } else if (drro->drr_type != doi.doi_type ||
> - drro->drr_blksz != doi.doi_data_block_size ||
> + drro->drr_blksz < doi.doi_data_block_size ||
> + (drro->drr_blksz > doi.doi_data_block_size &&
> + doi.doi_max_offset > doi.doi_data_block_size) ||
> drro->drr_bonustype != doi.doi_bonus_type ||
> drro->drr_bonuslen != doi.doi_bonus_size) {
> /* currently allocated, but with different properties */
> @@ -2210,6 +2213,10 @@ receive_object(struct receive_writer_arg *rwa, str
> err = dmu_object_reclaim(rwa->os, drro->drr_object,
> drro->drr_type, drro->drr_blksz,
> drro->drr_bonustype, drro->drr_bonuslen, tx);
> + } else if (drro->drr_blksz != doi.doi_data_block_size) {
> + /* currently allocated, just with one block of different
> size */
> + err = dmu_object_set_blocksize(rwa->os, drro->drr_object,
> + drro->drr_blksz, 0, tx);
> }
> if (err != 0) {
> dmu_tx_commit(tx);
>
>
> --
> Alexander Motin
>
> ------------------------------------------
> openzfs: openzfs-developer
> Permalink:
> https://openzfs.topicbox.com/groups/developer/T1be15fbaaa263b00-M8a12865ba14f30126f6200ca
> Delivery options:
> https://openzfs.topicbox.com/groups/developer/subscription
>
------------------------------------------
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/T1be15fbaaa263b00-M843039fe7fece3fedba90f91
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
