Page MenuHomeFreeBSD
Differential D13106

da(4): Short-circuit unnecessary BIO_FLUSH commands
ClosedPublic
Actions

Authored by asomers on Nov 15 2017, 4:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 3:28 PM
Unknown Object (File)
Mar 11 2025, 7:58 PM
Unknown Object (File)
Mar 8 2025, 11:22 PM
Unknown Object (File)
Feb 17 2025, 4:11 AM
Unknown Object (File)
Jan 10 2025, 5:00 AM
Unknown Object (File)
Jan 9 2025, 12:00 PM
Unknown Object (File)
Dec 23 2024, 7:25 AM
Unknown Object (File)
Oct 29 2024, 1:18 AM
View All 33 Files
Subscribers
gibbs
imp
ken

Details

Reviewers
scottl
imp
Commits
rS326779: MFC r326036:
rS326036: da(4): Short-circuit unnecessary BIO_FLUSH commands
Summary

da(4): Short-circuit unnecessary BIO_FLUSH commands

sys/cam/scsi/scsi_da.c
Complete BIO_FLUSH commands immediately if the da(4) device hasn't
been written since the last flush. If we haven't written to the
device, there is no reason to send a flush.




Submitted by:   gibbs
Test Plan
ZFS test suite
Diff Detail
Repository 
rS FreeBSD src repository - subversion 
Lint 
Lint Passed
 
Unit 
No Test Coverage
 
Build Status 
Buildable 12760
Build 13028: arc lint + arc unit
 
Event Timeline
asomers created this revision.Nov 15 2017, 4:57 PM
Herald added a subscriber: imp.  ·  View Herald TranscriptNov 15 2017, 4:57 PM
Harbormaster completed remote builds in B12760: Diff 35285.Nov 15 2017, 4:57 PM
imp added a comment.Nov 15 2017, 5:00 PM
Same thing for the ata version?



Otherwise seems sane.
asomers added a comment.Nov 15 2017, 6:03 PM


In D13106#272468, @imp wrote:


Same thing for the ata version?



Otherwise seems sane.





adastart is not totally symmetric with dastart.  My naive attempt to port the change is causing deadlocks.  Unless somebody else wants to try, I'm inclined to commit it just for da.
asomers added a comment.Nov 20 2017, 10:17 PM
@imp, do you approve this revision?
imp accepted this revision.Nov 20 2017, 10:21 PM
As far as I can tell, this rev is fine. I get the ada issues and while I would like to see the same sort of thing done there, I accept that it's harder.
This revision is now accepted and ready to land.Nov 20 2017, 10:21 PM
Closed by commit rS326036: da(4): Short-circuit unnecessary BIO_FLUSH commands (authored by asomers).  ·  Explain WhyNov 20 2017, 10:27 PM
This revision was automatically updated to reflect the committed changes.
Revision Contents
Changeset List
PathSize
sys/
cam/
scsi/
21 lines
Diff 35285
View Options
sys/cam/scsi/scsi_da.c
Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
 *
 *

 * @lldev Per-adapter data
 * @lldev Per-adapter data

 * @ifreqp Interface request
 * @ifreqp Interface request

 */
 */

int
int

xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)

{
{

        xge_hal_status_e status = XGE_HAL_OK;
        xge_hal_status_e status = XGE_HAL_OK;

        char *data = (char *)ifreqp->ifr_data;
        char tmpdata;

        char *data;

        void *info = NULL;
        void *info = NULL;

        int retValue = EINVAL;
        int retValue = EINVAL;




        tmpdata = fubyte(ifreqp->ifr_data);

        if (tmpdata == -1)

                return (EFAULT);

        data = &tmpdata;



        switch(*data) {
        switch(*data) {

jhbUnsubmitted
Not Done
Inline Actions
If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:



char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {



Also, I wonder if we should prefer fuebyte() in new code instead?  (Maybe kib@ has an opinion on that)
jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions
data is used later or I would have done that.  We don't currently have fuebyte()
brooks: `data` is used later or I would have done that.  We don't currently have fuebyte()
            case XGE_QUERY_STATS:
            case XGE_QUERY_STATS:

                mtx_lock(&lldev->mtx_drv);
                mtx_lock(&lldev->mtx_drv);

                status = xge_hal_stats_hw(lldev->devh,
                status = xge_hal_stats_hw(lldev->devh,

                    (xge_hal_stats_hw_info_t **)&info);
                    (xge_hal_stats_hw_info_t **)&info);

                mtx_unlock(&lldev->mtx_drv);
                mtx_unlock(&lldev->mtx_drv);

                if(status == XGE_HAL_OK) {
                if(status == XGE_HAL_OK) {

                    if(copyout(info, ifreqp->ifr_data,
                    if(copyout(info, ifreqp->ifr_data,

                        sizeof(xge_hal_stats_hw_info_t)) == 0)
                        sizeof(xge_hal_stats_hw_info_t)) == 0)

▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines                if(copyout(&lldev->driver_stats, ifreqp->ifr_data,

                    sizeof(int)) == 0)
                    sizeof(int)) == 0)

                    retValue = 0;
                    retValue = 0;

                break;
                break;




            case XGE_SET_BUFFER_MODE_1:
            case XGE_SET_BUFFER_MODE_1:

            case XGE_SET_BUFFER_MODE_2:
            case XGE_SET_BUFFER_MODE_2:

            case XGE_SET_BUFFER_MODE_5:
            case XGE_SET_BUFFER_MODE_5:

                *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
                *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';

                if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
                if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)

jhbUnsubmitted
Done
Inline Actions
So this is the only place that uses 'data' and it is wrong (sizeof is wrong).   I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().
jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong).   I would…
                    retValue = 0;
                    retValue = 0;

                break;
                break;

            default:
            default:

                xge_trace(XGE_TRACE, "Nothing is matching");
                xge_trace(XGE_TRACE, "Nothing is matching");

                retValue = ENOTTY;
                retValue = ENOTTY;

                break;
                break;

        }
        }

        return retValue;
        return retValue;

}
}




/**
/**

 * xge_ioctl_registers
 * xge_ioctl_registers

 * IOCTL to get registers
 * IOCTL to get registers

 *
 *

 * @lldev Per-adapter data
 * @lldev Per-adapter data

 * @ifreqp Interface request
 * @ifreqp Interface request

 */
 */

int
int

xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)

{
{

        xge_register_t *data = (xge_register_t *)ifreqp->ifr_data;
        xge_register_t tmpdata;

        xge_register_t *data;

        xge_hal_status_e status = XGE_HAL_OK;
        xge_hal_status_e status = XGE_HAL_OK;

        int retValue = EINVAL, offset = 0, index = 0;
        int retValue = EINVAL, offset = 0, index = 0;

        int error;

        u64 val64 = 0;
        u64 val64 = 0;



        error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));

        if (error != 0)

                return (error);

        data = &tmpdata;




        /* Reading a register */
        /* Reading a register */

        if(strcmp(data->option, "-r") == 0) {
        if(strcmp(data->option, "-r") == 0) {

            data->value = 0x0000;
            data->value = 0x0000;

            mtx_lock(&lldev->mtx_drv);
            mtx_lock(&lldev->mtx_drv);

            status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
            status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,

                &data->value);
                &data->value);

            mtx_unlock(&lldev->mtx_drv);
            mtx_unlock(&lldev->mtx_drv);

            if(status == XGE_HAL_OK) {
            if(status == XGE_HAL_OK) {

                if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
                if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)

jhbUnsubmitted
Not Done
Inline Actions
The fact that it does copyout() already makes this function even more amusing.
jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions
The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.



This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.
brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
                    retValue = 0;
                    retValue = 0;

            }
            }

        }
        }

        /* Writing to a register */
        /* Writing to a register */

        else if(strcmp(data->option, "-w") == 0) {
        else if(strcmp(data->option, "-w") == 0) {

            mtx_lock(&lldev->mtx_drv);
            mtx_lock(&lldev->mtx_drv);

            status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
            status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,

                data->value);
                data->value);

▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines