Alok Aggarwal wrote: > On Wed, 5 Sep 2007, Moinak Ghosh wrote: > > >> Updated webrevs are put up at: >> >> http://www.genunix.org/distributions/belenix_site//binfiles/hsfswebrev/index.htm >> > > I've got a few comments. > > hsfs_vnops.c: the two avl comparison helpers > hsched_deadline_compare() and hsched_offset_compare() > are declared as static but not defined as static. Needs > to be changed. Also both the functions resort to > checking 'hio' pointers if timestamp/blkno is equal - > I don't think this accomplishes anything and should be > removed. >
I will fix the static definition. This code was lifted from ZFS, vdev_queue.c where they have that pointer comparison. I think the pointer comparison is there to provide a protection in the case there are two requests that affect the same block. Trying to enqueue a duplicate request in the avl tree results in a panic. Pointer comparison avoids that problem since the memory allocated to the two duplicate bufs will be different. In my hsfs changes, this also avoids a corner case problem with the sentinel. Say one round of reads is over and we have a sentinel sitting in the tree. At some later time another read for the same block comes down, because the page has expired from the cache. Doing a pointer comparison will avoid an avl_tree panic. > hsfs_vnops.c: line 1933: this allows for an fsid bigger > than the maxsize of an int (which is what is passed in). > probably reduce the size to 23 instead and add a comment > noting the reasoning behind it. > hsfs computes the fsid as an int. > hsfs_vnops.c: hsched_init() - the cmn_errs don't > convey any useful information here. Perhaps emit a > single error message indicating quering ldi for > maxtransfer failed, resorting to 16k default. > Sometime you may want to know why something failed and a generic error message does not convey any helpful debug information. That is why I put different cmn_errs that print different messages and error codes for different failures. But I think I can make the error messages a little more verbose. > hsfs_vnops.c: hsched_enqueue_io() - make it static > Yes. > hsfs_vnops.c: It doesn't look like hsfs keeps any kstats > around, it would have been useful to keep stats around > for things like readahead and coalesced reads; or, even > deadline scheduling. Does it make any sense to add static > dtrace probes to sections of code that do read coalescing > and deadline reads? > Yes these will be added. This was the feedback from Frank as well and it appears that private debugging/observability kstats and SDT probes do not require an ARC review. Regards, Moinak. > Alok > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
